From 09cb647b5a6a3fcfcfcbc003fb2e823e780b3ab1 Mon Sep 17 00:00:00 2001 From: mansiverma897993 Date: Sat, 20 Jun 2026 13:50:28 +0530 Subject: [PATCH 1/2] Fix unsoundness in JsValueStore by replacing unsafe raw pointer mutation of Arc with safe OnceLock --- core/wintertc/src/store/from.rs | 28 ++++++++------------------ core/wintertc/src/store/mod.rs | 35 ++++++++------------------------- core/wintertc/src/store/to.rs | 5 +---- 3 files changed, 17 insertions(+), 51 deletions(-) diff --git a/core/wintertc/src/store/from.rs b/core/wintertc/src/store/from.rs index 203746fd042..3e369f0c6c4 100644 --- a/core/wintertc/src/store/from.rs +++ b/core/wintertc/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/wintertc/src/store/mod.rs b/core/wintertc/src/store/mod.rs index 68b8b699e8e..e0a1e34ee75 100644 --- a/core/wintertc/src/store/mod.rs +++ b/core/wintertc/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/wintertc/src/store/to.rs b/core/wintertc/src/store/to.rs index 9ff24570c67..998f6c9fd3a 100644 --- a/core/wintertc/src/store/to.rs +++ b/core/wintertc/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)), From 357b23d64961869634715e7956f37de50ab45956 Mon Sep 17 00:00:00 2001 From: mansiverma897993 Date: Mon, 24 Aug 2026 13:19:49 +0530 Subject: [PATCH 2/2] chore: fix clippy lints for the Rust 1.98 toolchain Fix the unused-async-trait-impl and useless-format clippy lints introduced by the Rust 1.98 stable toolchain in existing ModuleLoader and Fetcher implementations. --- core/engine/src/module/loader/mod.rs | 8 +- core/engine/src/vm/mod.rs | 2 +- core/engine/tests/module.rs | 102 +++++++++++++---------- core/runtime/src/fetch/fetchers.rs | 95 +++++++++++---------- core/runtime/src/fetch/tests/e2e.rs | 14 ++-- core/runtime/src/fetch/tests/mod.rs | 22 ++--- core/runtime/src/fetch/tests/response.rs | 5 +- 7 files changed, 132 insertions(+), 116 deletions(-) diff --git a/core/engine/src/module/loader/mod.rs b/core/engine/src/module/loader/mod.rs index 21b0ed06b2f..bb153599471 100644 --- a/core/engine/src/module/loader/mod.rs +++ b/core/engine/src/module/loader/mod.rs @@ -205,15 +205,15 @@ pub trait ModuleLoader: Any { pub struct IdleModuleLoader; impl ModuleLoader for IdleModuleLoader { - async fn load_imported_module( + fn load_imported_module( self: Rc, _referrer: Referrer, _request: ModuleRequest, _context: &RefCell<&mut Context>, - ) -> JsResult { - Err(JsNativeError::typ() + ) -> impl Future> { + std::future::ready(Err(JsNativeError::typ() .with_message("module resolution is disabled for this context") - .into()) + .into())) } } diff --git a/core/engine/src/vm/mod.rs b/core/engine/src/vm/mod.rs index dc6b1dca985..ffa2a83d151 100644 --- a/core/engine/src/vm/mod.rs +++ b/core/engine/src/vm/mod.rs @@ -756,7 +756,7 @@ impl Context { println!( "{:, _referrer: Referrer, request: boa_engine::module::ModuleRequest, context: &RefCell<&mut Context>, - ) -> JsResult { - assert_eq!(request.specifier().to_std_string_escaped(), "basic"); - let src = self.0.clone(); + ) -> impl Future> { + std::future::ready({ + assert_eq!(request.specifier().to_std_string_escaped(), "basic"); + let src = self.0.clone(); - Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + }) } } @@ -66,22 +68,24 @@ fn test_json_module_from_str() { fn test_json_module_dynamic_import() { struct TestModuleLoader(JsString); impl ModuleLoader for TestModuleLoader { - async fn load_imported_module( + fn load_imported_module( self: Rc, _referrer: Referrer, request: boa_engine::module::ModuleRequest, context: &RefCell<&mut Context>, - ) -> JsResult { - assert_eq!(request.specifier().to_std_string_escaped(), "basic"); - - // Verify attributes were passed correctly - let type_attr = request - .get_attribute("type") - .expect("should have type attribute"); - assert_eq!(type_attr.to_std_string_escaped(), "json"); - - let src = self.0.clone(); - Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + ) -> impl Future> { + std::future::ready({ + assert_eq!(request.specifier().to_std_string_escaped(), "basic"); + + // Verify attributes were passed correctly + let type_attr = request + .get_attribute("type") + .expect("should have type attribute"); + assert_eq!(type_attr.to_std_string_escaped(), "json"); + + let src = self.0.clone(); + Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + }) } } @@ -148,21 +152,23 @@ fn test_json_module_dynamic_import() { fn test_json_module_static_import_with_attributes() { struct TestModuleLoader(JsString); impl ModuleLoader for TestModuleLoader { - async fn load_imported_module( + fn load_imported_module( self: Rc, _referrer: Referrer, request: boa_engine::module::ModuleRequest, context: &RefCell<&mut Context>, - ) -> JsResult { - assert_eq!(request.specifier().to_std_string_escaped(), "basic"); - - let type_attr = request - .get_attribute("type") - .expect("should have type attribute"); - assert_eq!(type_attr.to_std_string_escaped(), "json"); - - let src = self.0.clone(); - Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + ) -> impl Future> { + std::future::ready({ + assert_eq!(request.specifier().to_std_string_escaped(), "basic"); + + let type_attr = request + .get_attribute("type") + .expect("should have type attribute"); + assert_eq!(type_attr.to_std_string_escaped(), "json"); + + let src = self.0.clone(); + Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + }) } } @@ -203,21 +209,23 @@ fn test_json_module_static_import_with_attributes() { fn test_json_module_reexport_with_attributes() { struct TestModuleLoader(JsString); impl ModuleLoader for TestModuleLoader { - async fn load_imported_module( + fn load_imported_module( self: Rc, _referrer: Referrer, request: boa_engine::module::ModuleRequest, context: &RefCell<&mut Context>, - ) -> JsResult { - assert_eq!(request.specifier().to_std_string_escaped(), "basic"); - - let type_attr = request - .get_attribute("type") - .expect("should have type attribute"); - assert_eq!(type_attr.to_std_string_escaped(), "json"); - - let src = self.0.clone(); - Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + ) -> impl Future> { + std::future::ready({ + assert_eq!(request.specifier().to_std_string_escaped(), "basic"); + + let type_attr = request + .get_attribute("type") + .expect("should have type attribute"); + assert_eq!(type_attr.to_std_string_escaped(), "json"); + + let src = self.0.clone(); + Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + }) } } @@ -367,19 +375,21 @@ fn test_dynamic_import_non_string_attribute_value() { fn test_dynamic_import_symbol_key() { struct TestModuleLoader(JsString); impl ModuleLoader for TestModuleLoader { - async fn load_imported_module( + fn load_imported_module( self: Rc, _referrer: Referrer, request: boa_engine::module::ModuleRequest, context: &RefCell<&mut Context>, - ) -> JsResult { - assert_eq!(request.specifier().to_std_string_escaped(), "basic"); + ) -> impl Future> { + std::future::ready({ + assert_eq!(request.specifier().to_std_string_escaped(), "basic"); - // Verify attributes were passed correctly (symbol key should be ignored) - assert!(request.get_attribute("type").is_none()); + // Verify attributes were passed correctly (symbol key should be ignored) + assert!(request.get_attribute("type").is_none()); - let src = self.0.clone(); - Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + let src = self.0.clone(); + Ok(Module::parse_json(src, &mut context.borrow_mut()).unwrap()) + }) } } diff --git a/core/runtime/src/fetch/fetchers.rs b/core/runtime/src/fetch/fetchers.rs index 12f017bb5a6..a56c83a9c2d 100644 --- a/core/runtime/src/fetch/fetchers.rs +++ b/core/runtime/src/fetch/fetchers.rs @@ -12,13 +12,15 @@ use std::rc::Rc; pub struct ErrorFetcher; impl Fetcher for ErrorFetcher { - async fn fetch( + fn fetch( self: Rc, _request: JsRequest, _signal: Option, _context: &RefCell<&mut Context>, - ) -> JsResult { - Err(js_error!(ReferenceError: "ErrorFetcher used in fetch API.")) + ) -> impl Future> { + std::future::ready(Err(js_error!( + ReferenceError: "ErrorFetcher used in fetch API." + ))) } } @@ -32,60 +34,63 @@ pub struct BlockingReqwestFetcher { #[cfg(feature = "reqwest-blocking")] impl Fetcher for BlockingReqwestFetcher { - async fn fetch( + fn fetch( self: Rc, request: JsRequest, signal: Option, _context: &RefCell<&mut Context>, - ) -> JsResult { - use boa_engine::{JsError, JsString}; + ) -> impl Future> { + let result = (|| { + use boa_engine::{JsError, JsString}; - if let Some(ref sig) = signal - && let Some(sig_ref) = sig.downcast_ref::() - && sig_ref.is_aborted() - { - return Err(JsError::from_opaque( - boa_engine::js_string!("AbortError").into(), - )); - } + if let Some(ref sig) = signal + && let Some(sig_ref) = sig.downcast_ref::() + && sig_ref.is_aborted() + { + return Err(JsError::from_opaque( + boa_engine::js_string!("AbortError").into(), + )); + } - let request = request.into_inner(); - let url = request.uri().to_string(); - let req = self - .client - .request(request.method().clone(), &url) - .headers(request.headers().clone()); + let request = request.into_inner(); + let url = request.uri().to_string(); + let req = self + .client + .request(request.method().clone(), &url) + .headers(request.headers().clone()); - let req = req - .body(request.body().clone()) - .build() - .map_err(JsError::from_rust)?; + let req = req + .body(request.body().clone()) + .build() + .map_err(JsError::from_rust)?; - let resp = self.client.execute(req).map_err(JsError::from_rust)?; + let resp = self.client.execute(req).map_err(JsError::from_rust)?; - if let Some(ref sig) = signal - && let Some(sig_ref) = sig.downcast_ref::() - && sig_ref.is_aborted() - { - return Err(JsError::from_opaque( - boa_engine::js_string!("AbortError").into(), - )); - } + if let Some(ref sig) = signal + && let Some(sig_ref) = sig.downcast_ref::() + && sig_ref.is_aborted() + { + return Err(JsError::from_opaque( + boa_engine::js_string!("AbortError").into(), + )); + } - let status = resp.status(); - let headers = resp.headers().clone(); - let bytes = resp.bytes().map_err(JsError::from_rust)?; - let mut builder = http::Response::builder().status(status.as_u16()); + let status = resp.status(); + let headers = resp.headers().clone(); + let bytes = resp.bytes().map_err(JsError::from_rust)?; + let mut builder = http::Response::builder().status(status.as_u16()); - for k in headers.keys() { - for v in headers.get_all(k) { - builder = builder.header(k.as_str(), v); + for k in headers.keys() { + for v in headers.get_all(k) { + builder = builder.header(k.as_str(), v); + } } - } - builder - .body(bytes.to_vec()) - .map_err(JsError::from_rust) - .map(|request| JsResponse::basic(JsString::from(url), request)) + builder + .body(bytes.to_vec()) + .map_err(JsError::from_rust) + .map(|request| JsResponse::basic(JsString::from(url), request)) + })(); + async { result } } } diff --git a/core/runtime/src/fetch/tests/e2e.rs b/core/runtime/src/fetch/tests/e2e.rs index 551483e781d..e08326c158b 100644 --- a/core/runtime/src/fetch/tests/e2e.rs +++ b/core/runtime/src/fetch/tests/e2e.rs @@ -43,16 +43,18 @@ impl E2eFetcher { } impl crate::fetch::Fetcher for E2eFetcher { - async fn fetch( + fn fetch( self: Rc, request: JsRequest, _signal: Option, context: &RefCell<&mut Context>, - ) -> JsResult { - match request.uri().path() { - "/headers" => Self::headers(&request, &mut context.borrow_mut()), - _ => Err(js_error!("Invalid request.")), - } + ) -> impl Future> { + std::future::ready({ + match request.uri().path() { + "/headers" => Self::headers(&request, &mut context.borrow_mut()), + _ => Err(js_error!("Invalid request.")), + } + }) } } diff --git a/core/runtime/src/fetch/tests/mod.rs b/core/runtime/src/fetch/tests/mod.rs index a6f6de85260..a2839e79e26 100644 --- a/core/runtime/src/fetch/tests/mod.rs +++ b/core/runtime/src/fetch/tests/mod.rs @@ -38,19 +38,21 @@ impl TestFetcher { } impl crate::fetch::Fetcher for TestFetcher { - async fn fetch( + fn fetch( self: Rc, request: JsRequest, _signal: Option, _context: &RefCell<&mut Context>, - ) -> JsResult { - let request = request.into_inner(); - self.requests_received.borrow_mut().push(request.clone()); - let url = request.uri(); - self.request_mapper - .get(url) - .cloned() - .map(|response| JsResponse::basic(JsString::from(url.to_string()), response)) - .ok_or_else(|| js_error!("No response found for URL")) + ) -> impl Future> { + std::future::ready({ + let request = request.into_inner(); + self.requests_received.borrow_mut().push(request.clone()); + let url = request.uri(); + self.request_mapper + .get(url) + .cloned() + .map(|response| JsResponse::basic(JsString::from(url.to_string()), response)) + .ok_or_else(|| js_error!("No response found for URL")) + }) } } diff --git a/core/runtime/src/fetch/tests/response.rs b/core/runtime/src/fetch/tests/response.rs index c49a6d18601..b5e808895f2 100644 --- a/core/runtime/src/fetch/tests/response.rs +++ b/core/runtime/src/fetch/tests/response.rs @@ -108,10 +108,7 @@ fn response_json() { TestAction::inspect_context(|ctx| { let response = ctx.global_object().get(js_str!("response"), ctx).unwrap(); let response = response.as_promise().unwrap().await_blocking(ctx).unwrap(); - assert_eq!( - format!("{}", response.display_obj(false)), - "{\n hello world: 123\n}" - ); + assert_eq!(response.display_obj(false), "{\n hello world: 123\n}"); }), ]); }