Fix unsoundness in JsValueStore by replacing unsafe raw pointer mutation of Arc with safe OnceLock

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::<LitStr>()?.value();
                     Ok(())
+                } else if meta.path.is_ident("into_js_with") {
+                    let _unused = meta.value()?.parse::<LitStr>()?;
+                    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::<LitStr>()?.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::<LitStr>()?;
+                    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)]` \
                             ",

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<JsValueStore> {
     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<JsValueStore> {
     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)
 }
 

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<StringStore> 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<ValueStoreInner>);
+pub struct JsValueStore(Arc<OnceLock<ValueStoreInner>>);
 
 impl TryIntoJs for JsValueStore {
     fn try_into_js(&self, context: &mut Context) -> JsResult<JsValue> {
@@ -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

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)),

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<JsValue> {
+    Ok(JsValue::new(*value))
+}
+
+fn my_custom_converter_from_js(value: &JsValue, _context: &mut Context) -> JsResult<i32> {
+    value.try_js_into(_context)
+}
+
+fn main() {}