From bc46902ba1e4c48e762a64072739cbe7c0f060cc Mon Sep 17 00:00:00 2001 From: mansiverma897993 Date: Fri, 19 Jun 2026 13:00:39 +0530 Subject: [PATCH 1/3] fix: allow deriving both TryFromJs and TryIntoJs with custom converters --- core/macros/src/lib.rs | 24 +++++++++++++++++------- tests/macros/tests/derive/both.rs | 23 +++++++++++++++++++++++ 2 files changed, 40 insertions(+), 7 deletions(-) create mode 100644 tests/macros/tests/derive/both.rs diff --git a/core/macros/src/lib.rs b/core/macros/src/lib.rs index f53ac93b708..0d3579b5f48 100644 --- a/core/macros/src/lib.rs +++ b/core/macros/src/lib.rs @@ -524,10 +524,10 @@ fn generate_conversion( let mut from_js_with = None; let mut field_name = rename.rename(format!("{name}")); - if let Some(attr) = field + for attr in field .attrs .into_iter() - .find(|attr| attr.path().is_ident("boa")) + .filter(|attr| attr.path().is_ident("boa")) { attr.parse_nested_meta(|meta| { if meta.path.is_ident("from_js_with") { @@ -538,11 +538,18 @@ fn generate_conversion( let value = meta.value()?; field_name = value.parse::()?.value(); Ok(()) + } else if meta.path.is_ident("into_js_with") { + let _unused = meta.value()?.parse::()?; + Ok(()) + } else if meta.path.is_ident("skip") && meta.input.is_empty() { + Ok(()) } else { Err(meta.error( "invalid syntax in the `#[boa()]` attribute. \ - Note that this attribute only accepts the following syntax: \ - `#[boa(from_js_with = \"fully::qualified::path\")]`", + Note that this attribute only accepts the following syntax: \ + \n* `#[boa(from_js_with = \"fully::qualified::path\")]` \ + \n* `#[boa(rename = \"jsPropertyName\")]` \ + ", )) } }) @@ -666,14 +673,17 @@ fn generate_obj_properties( let value = meta.value()?; prop_key = value.parse::()?.value(); Ok(()) - } else if meta.path.is_ident("skip") & meta.input.is_empty() { + } else if meta.path.is_ident("skip") && meta.input.is_empty() { skip = true; Ok(()) + } else if meta.path.is_ident("from_js_with") { + let _unused = meta.value()?.parse::()?; + Ok(()) } else { Err(meta.error( "invalid syntax in the `#[boa()]` attribute. \ - Note that this attribute only accepts the following syntax: \ - \n* `#[boa(into_js_with = \"fully::qualified::path\")]`\ + Note that this attribute only accepts the following syntax: \ + \n* `#[boa(into_js_with = \"fully::qualified::path\")]` \ \n* `#[boa(rename = \"jsPropertyName\")]` \ \n* `#[boa(skip)]` \ ", diff --git a/tests/macros/tests/derive/both.rs b/tests/macros/tests/derive/both.rs new file mode 100644 index 00000000000..bf270efa636 --- /dev/null +++ b/tests/macros/tests/derive/both.rs @@ -0,0 +1,23 @@ +#![allow(unused)] + +use boa_engine::{ + value::{TryFromJs, TryIntoJs}, + Context, JsResult, JsValue, +}; + +#[derive(TryFromJs, TryIntoJs)] +struct Blah { + #[boa(into_js_with = "my_custom_converter_to_js")] + #[boa(from_js_with = "my_custom_converter_from_js")] + x: i32, +} + +fn my_custom_converter_to_js(value: &i32, _context: &mut Context) -> JsResult { + Ok(JsValue::new(*value)) +} + +fn my_custom_converter_from_js(value: &JsValue, _context: &mut Context) -> JsResult { + value.try_js_into(_context) +} + +fn main() {} From 6be840340d15d25f64cde808b7304d1e16034924 Mon Sep 17 00:00:00 2001 From: mansiverma897993 Date: Fri, 19 Jun 2026 13:54:41 +0530 Subject: [PATCH 2/3] chore: trigger workflow re-run From 15efcccd5b510d1661538c3f6e7a8971f0e16f63 Mon Sep 17 00:00:00 2001 From: mansiverma897993 Date: Sat, 20 Jun 2026 13:50:28 +0530 Subject: [PATCH 3/3] Fix unsoundness in JsValueStore by replacing unsafe raw pointer mutation of Arc with safe OnceLock --- core/runtime/src/store/from.rs | 28 ++++++++------------------- core/runtime/src/store/mod.rs | 35 ++++++++-------------------------- core/runtime/src/store/to.rs | 5 +---- 3 files changed, 17 insertions(+), 51 deletions(-) diff --git a/core/runtime/src/store/from.rs b/core/runtime/src/store/from.rs index 203746fd042..3e369f0c6c4 100644 --- a/core/runtime/src/store/from.rs +++ b/core/runtime/src/store/from.rs @@ -86,7 +86,7 @@ fn try_from_array_clone( // Create an empty clone, we will replace its inner values after we gather them. // To stop the recursion, we need to add the right value to the seen map prior, // though. - let mut dolly = JsValueStore::empty(); + let dolly = JsValueStore::empty(); seen.insert(&JsObject::from(array.clone()), dolly.clone()); let length = array.length(context)?; @@ -106,10 +106,7 @@ fn try_from_array_clone( } } - // SAFETY: This is safe as this function is the sole owner of the store. - unsafe { - dolly.replace(ValueStoreInner::Array(inner)); - } + dolly.replace(ValueStoreInner::Array(inner)); Ok(dolly) } @@ -189,7 +186,7 @@ fn try_from_map( context: &mut Context, ) -> JsResult { let mut new_map = Vec::new(); - let mut store = JsValueStore::new(ValueStoreInner::Empty); + let store = JsValueStore::empty(); seen.insert(original, store.clone()); map.for_each_native(|k, v| { @@ -200,10 +197,7 @@ fn try_from_map( Ok(()) })?; - // SAFETY: This is safe as this function is the sole owner of the store. - unsafe { - store.replace(ValueStoreInner::Map(new_map)); - } + store.replace(ValueStoreInner::Map(new_map)); Ok(store) } @@ -216,7 +210,7 @@ fn try_from_set( context: &mut Context, ) -> JsResult { let mut new_set = Vec::new(); - let mut store = JsValueStore::new(ValueStoreInner::Empty); + let store = JsValueStore::empty(); seen.insert(original, store.clone()); set.for_each_native(|v| { @@ -226,10 +220,7 @@ fn try_from_set( Ok(()) })?; - // SAFETY: This is safe as this function is the sole owner of the store. - unsafe { - store.replace(ValueStoreInner::Set(new_set)); - } + store.replace(ValueStoreInner::Set(new_set)); Ok(store) } @@ -271,7 +262,7 @@ fn try_from_js_object_clone( // Create a new object and add own properties to it. This does not preserve // the prototype (nor do we want to). - let mut dolly = JsValueStore::empty(); + let dolly = JsValueStore::empty(); seen.insert(object, dolly.clone()); let keys = object.own_property_keys(context)?; @@ -288,10 +279,7 @@ fn try_from_js_object_clone( fields.push((key, v)); } - // SAFETY: This is safe as this function is the sole owner of the store. - unsafe { - dolly.replace(ValueStoreInner::Object(fields)); - } + dolly.replace(ValueStoreInner::Object(fields)); Ok(dolly) } diff --git a/core/runtime/src/store/mod.rs b/core/runtime/src/store/mod.rs index 68b8b699e8e..e0a1e34ee75 100644 --- a/core/runtime/src/store/mod.rs +++ b/core/runtime/src/store/mod.rs @@ -6,7 +6,7 @@ use boa_engine::builtins::typed_array::TypedArrayKind; use boa_engine::value::TryIntoJs; use boa_engine::{Context, JsError, JsResult, JsString, JsValue, JsVariant, js_error}; use rustc_hash::FxHashSet; -use std::sync::Arc; +use std::sync::{Arc, OnceLock}; mod from; mod to; @@ -50,11 +50,6 @@ impl From for JsString { /// Inner value for [`JsValueStore`]. #[derive(Debug)] enum ValueStoreInner { - /// An Empty value that will be filled later. This is only used during - /// construction, and if encountered at other points will result - /// in an error. - Empty, - /// Primitive values - `null`. Null, @@ -147,7 +142,7 @@ enum ValueStoreInner { /// /// [sca]: https://developer.mozilla.org/en-US/docs/Web/API/Web_Workers_API/Structured_clone_algorithm #[derive(Debug, Clone)] -pub struct JsValueStore(Arc); +pub struct JsValueStore(Arc>); impl TryIntoJs for JsValueStore { fn try_into_js(&self, context: &mut Context) -> JsResult { @@ -162,33 +157,19 @@ impl JsValueStore { /// (to allow for recursive data). Therefore, the pattern is to create the /// store with an empty inner, then create the sub-content, and replace the /// empty inner with the new inner. - /// - /// # SAFETY - /// This should only be done if the inner content is [`ValueStoreInner::Empty`], - /// and only by the creator of the current [`JsValueStore`]. We enforce the first - /// rule at runtime (and will panic), and the second rule by requiring a mutable - /// reference. This is still unsafe and relies on unsafe pointer access. - unsafe fn replace(&mut self, other: ValueStoreInner) { - let ptr = Arc::as_ptr(&self.0).cast_mut(); - - assert!(!ptr.is_null()); - unsafe { - assert!( - matches!(*ptr, ValueStoreInner::Empty), - "ValueStoreInner must be empty." - ); - - *ptr = other; - } + fn replace(&self, other: ValueStoreInner) { + self.0.set(other).expect("ValueStoreInner must be empty."); } /// A still-being-constructed value. fn empty() -> Self { - Self(Arc::new(ValueStoreInner::Empty)) + Self(Arc::new(OnceLock::new())) } fn new(inner: ValueStoreInner) -> Self { - Self(Arc::new(inner)) + let cell = OnceLock::new(); + cell.set(inner).expect("ValueStoreInner must be empty."); + Self(Arc::new(cell)) } /// Create a context-free [`JsValue`] equivalent from an existing `JsValue` and the diff --git a/core/runtime/src/store/to.rs b/core/runtime/src/store/to.rs index 9ff24570c67..998f6c9fd3a 100644 --- a/core/runtime/src/store/to.rs +++ b/core/runtime/src/store/to.rs @@ -192,10 +192,7 @@ pub(super) fn try_value_into_js( } // Match the value - match &*store.0 { - ValueStoreInner::Empty => { - unreachable!("ValueStoreInner::Empty should not exist after storage."); - } + match store.0.get().expect("ValueStoreInner must be initialized") { ValueStoreInner::Null => Ok(JsValue::null()), ValueStoreInner::Undefined => Ok(JsValue::undefined()), ValueStoreInner::Boolean(b) => Ok(JsValue::from(*b)),