From d7452d3068ece540d37add9fae795a43674238ed Mon Sep 17 00:00:00 2001 From: Felipe Armoni Date: Thu, 26 Mar 2026 10:22:00 -0300 Subject: [PATCH 1/5] Handle critical section comparisons in the main thread --- .../structured_data/atomics_object.rs | 62 +++++++------------ .../src/ecmascript/types/spec/data_block.rs | 1 - 2 files changed, 23 insertions(+), 40 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs index 506817463..4e32d8898 100644 --- a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs +++ b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs @@ -1485,8 +1485,23 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( // }. // 28. Perform AddWaiter(WL, waiterRecord). // 29. If mode is sync, then + + let data_block = buffer.get_data_block(agent); + + // Re-read value under critical section to avoid TOCTOU race. + let slot = data_block.as_racy_slice().slice_from(byte_index_in_buffer); + let v_changed = if IS_I64 { + let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; + v as u64 != slot.load(Ordering::SeqCst) + } else { + let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; + v as i32 as u32 != slot.load(Ordering::SeqCst) + }; + if v_changed { + return BUILTIN_STRING_MEMORY.not_equal.into(); + } + if !IS_ASYNC { - let data_block = buffer.get_data_block(agent); // SAFETY: buffer is a valid SharedArrayBuffer and cannot be detached. A 0-sized SAB would // have a dangling data block, but Atomics.wait requires `byteIndex` to be within bounds, // so a 0-sized SAB would have been rejected earlier with a RangeError. @@ -1494,19 +1509,6 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( let waiter_record = WaiterRecord::new_shared(); let mut guard = waiters.lock().unwrap(); - // Re-read value under critical section to avoid TOCTOU race. - let slot = data_block.as_racy_slice().slice_from(byte_index_in_buffer); - let v_changed = if IS_I64 { - let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; - v as u64 != slot.load(Ordering::SeqCst) - } else { - let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; - v as i32 as u32 != slot.load(Ordering::SeqCst) - }; - if v_changed { - return BUILTIN_STRING_MEMORY.not_equal.into(); - } - // a. Perform SuspendThisAgent(WL, waiterRecord). guard.push_to_list(byte_index_in_buffer, waiter_record.clone()); @@ -1532,12 +1534,12 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( let promise = Global::new(agent, promise_capability.promise.unbind()); // 30. Else if timeoutTime is finite, then // a. Perform EnqueueAtomicsWaitAsyncTimeoutJob(WL, waiterRecord). - let buffer = buffer.get_data_block(agent).clone(); + + let data_block_clone = buffer.get_data_block(agent).clone(); enqueue_atomics_wait_async_job::( agent, - buffer, + data_block_clone, byte_index_in_buffer, - v, t, promise, gc, @@ -1672,7 +1674,7 @@ impl WaitAsyncJob { // c. Perform LeaveCriticalSection(WL). let promise_capability = PromiseCapability::from_promise(promise, true); let result = match result { - WaitResult::Ok | WaitResult::NotEqual => BUILTIN_STRING_MEMORY.ok.into(), + WaitResult::Ok => BUILTIN_STRING_MEMORY.ok.into(), WaitResult::TimedOut => BUILTIN_STRING_MEMORY.timed_out.into(), }; unwrap_try(promise_capability.try_resolve(agent, result, gc)); @@ -1688,9 +1690,8 @@ impl WaitAsyncJob { /// unused. fn enqueue_atomics_wait_async_job( agent: &mut Agent, - buffer: SharedDataBlock, + data_block: SharedDataBlock, byte_index_in_buffer: usize, - v: i64, t: u64, promise: Global, gc: NoGcScope, @@ -1698,31 +1699,14 @@ fn enqueue_atomics_wait_async_job( // 1. Let timeoutJob be a new Job Abstract Closure with no parameters that // captures WL and waiterRecord and performs the following steps when // called: + // let data_block = data_block.clone(); let signal = Arc::new(AtomicBool::new(false)); - let s = signal.clone(); let handle = thread::spawn(move || { // SAFETY: buffer is a cloned SharedDataBlock; non-dangling. - let waiters = unsafe { buffer.get_or_init_waiters() }; + let waiters = unsafe { data_block.get_or_init_waiters() }; let waiter_record = WaiterRecord::new_shared(); let mut guard = waiters.lock().unwrap(); - // Re-check the value under the critical section. - let slot = buffer.as_racy_slice().slice_from(byte_index_in_buffer); - let v_not_equal = if IS_I64 { - let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; - v as u64 != slot.load(Ordering::SeqCst) - } else { - let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; - v as i32 as u32 != slot.load(Ordering::SeqCst) - }; - - // Signal the main thread that we have the lock and are about to sleep. - s.store(true, StdOrdering::Release); - - if v_not_equal { - return WaitResult::NotEqual; - } - guard.push_to_list(byte_index_in_buffer, waiter_record.clone()); if t == u64::MAX { diff --git a/nova_vm/src/ecmascript/types/spec/data_block.rs b/nova_vm/src/ecmascript/types/spec/data_block.rs index fee2a7c63..5efb6cfdd 100644 --- a/nova_vm/src/ecmascript/types/spec/data_block.rs +++ b/nova_vm/src/ecmascript/types/spec/data_block.rs @@ -480,7 +480,6 @@ impl WaiterRecord { pub(crate) enum WaitResult { Ok, TimedOut, - NotEqual, } #[cfg(feature = "shared-array-buffer")] From 1736244b4d995334e2ac70d81d5b231213f1cdbd Mon Sep 17 00:00:00 2001 From: Felipe Armoni Date: Tue, 7 Apr 2026 09:29:40 -0300 Subject: [PATCH 2/5] Fix timeout issue --- .../src/ecmascript/builtins/structured_data/atomics_object.rs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs index 4e32d8898..9d0165d31 100644 --- a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs +++ b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs @@ -1699,14 +1699,16 @@ fn enqueue_atomics_wait_async_job( // 1. Let timeoutJob be a new Job Abstract Closure with no parameters that // captures WL and waiterRecord and performs the following steps when // called: - // let data_block = data_block.clone(); let signal = Arc::new(AtomicBool::new(false)); + let s = signal.clone(); let handle = thread::spawn(move || { // SAFETY: buffer is a cloned SharedDataBlock; non-dangling. let waiters = unsafe { data_block.get_or_init_waiters() }; let waiter_record = WaiterRecord::new_shared(); let mut guard = waiters.lock().unwrap(); + // Signal the main thread that we have the lock and are about to sleep. + s.store(true, StdOrdering::Release); guard.push_to_list(byte_index_in_buffer, waiter_record.clone()); if t == u64::MAX { From ba33e2ee821a4a37c23b8bf9376be26ca0e0c44d Mon Sep 17 00:00:00 2001 From: Felipe Armoni Date: Thu, 16 Apr 2026 09:42:44 -0300 Subject: [PATCH 3/5] Minor change --- .../builtins/structured_data/atomics_object.rs | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs index 9d0165d31..75a934a74 100644 --- a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs +++ b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs @@ -1487,20 +1487,6 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( // 29. If mode is sync, then let data_block = buffer.get_data_block(agent); - - // Re-read value under critical section to avoid TOCTOU race. - let slot = data_block.as_racy_slice().slice_from(byte_index_in_buffer); - let v_changed = if IS_I64 { - let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; - v as u64 != slot.load(Ordering::SeqCst) - } else { - let slot = unsafe { slot.as_aligned::().unwrap_unchecked() }; - v as i32 as u32 != slot.load(Ordering::SeqCst) - }; - if v_changed { - return BUILTIN_STRING_MEMORY.not_equal.into(); - } - if !IS_ASYNC { // SAFETY: buffer is a valid SharedArrayBuffer and cannot be detached. A 0-sized SAB would // have a dangling data block, but Atomics.wait requires `byteIndex` to be within bounds, From 3584f29987e4a5df05c75614d206596cb62b9320 Mon Sep 17 00:00:00 2001 From: Felipe Armoni Date: Thu, 16 Apr 2026 19:57:04 -0300 Subject: [PATCH 4/5] Updated enter and leave critical section --- .../structured_data/atomics_object.rs | 48 +++++++++---------- 1 file changed, 22 insertions(+), 26 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs index 75a934a74..1a530432b 100644 --- a/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs +++ b/nova_vm/src/ecmascript/builtins/structured_data/atomics_object.rs @@ -5,10 +5,7 @@ use std::{ hint::assert_unchecked, ops::ControlFlow, - sync::{ - Arc, - atomic::{AtomicBool, Ordering as StdOrdering}, - }, + sync::Arc, thread::{self, JoinHandle}, time::Duration, }; @@ -1421,6 +1418,7 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( gc: NoGcScope<'gc, '_>, ) -> Value<'gc> { let slot = buffer.as_slice(agent).slice_from(byte_index_in_buffer); + let data_block = buffer.get_data_block(agent).clone(); // 14. Let WL be GetWaiterList(block, byteIndexInBuffer). // 15. If mode is sync, then // a. Let promiseCapability be blocking. @@ -1429,6 +1427,14 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( // a. Let promiseCapability be ! NewPromiseCapability(%Promise%). // b. Let resultObject be OrdinaryObjectCreate(%Object.prototype%). // 17. Perform EnterCriticalSection(WL). + + // SAFETY: buffer is a valid SharedArrayBuffer and cannot be detached. A 0-sized SAB would + // have a dangling data block, but Atomics.wait requires `byteIndex` to be within bounds, + // so a 0-sized SAB would have been rejected earlier with a RangeError. + let waiters = unsafe { data_block.get_or_init_waiters() }; + let waiter_record = WaiterRecord::new_shared(); + let mut guard = waiters.lock().unwrap(); + // 18. Let elementType be TypedArrayElementType(typedArray). // 19. Let w be GetValueFromBuffer(buffer, byteIndexInBuffer, elementType, true, seq-cst). let v_not_equal_to_w = if IS_I64 { @@ -1447,6 +1453,8 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( // 20. If v ≠ w, then if v_not_equal_to_w { // a. Perform LeaveCriticalSection(WL). + drop(guard); + // b. If mode is sync, return "not-equal". if !IS_ASYNC { return BUILTIN_STRING_MEMORY.not_equal.into(); @@ -1464,6 +1472,7 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( // timeouts. Asynchronous immediate timeouts have special handling // in order to fail fast and avoid unnecessary Promise jobs. // b. Perform LeaveCriticalSection(WL). + drop(guard); // c. Perform ! CreateDataPropertyOrThrow(resultObject, "async", false). // d. Perform ! CreateDataPropertyOrThrow(resultObject, "value", "timed-out"). let result_object = @@ -1484,20 +1493,10 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( // [[Result]]: "ok" // }. // 28. Perform AddWaiter(WL, waiterRecord). + guard.push_to_list(byte_index_in_buffer, waiter_record.clone()); // 29. If mode is sync, then - - let data_block = buffer.get_data_block(agent); if !IS_ASYNC { - // SAFETY: buffer is a valid SharedArrayBuffer and cannot be detached. A 0-sized SAB would - // have a dangling data block, but Atomics.wait requires `byteIndex` to be within bounds, - // so a 0-sized SAB would have been rejected earlier with a RangeError. - let waiters = unsafe { data_block.get_or_init_waiters() }; - let waiter_record = WaiterRecord::new_shared(); - let mut guard = waiters.lock().unwrap(); - // a. Perform SuspendThisAgent(WL, waiterRecord). - guard.push_to_list(byte_index_in_buffer, waiter_record.clone()); - if t == u64::MAX { waiter_record.wait(guard); } else { @@ -1521,16 +1520,20 @@ fn do_wait_critical<'gc, const IS_ASYNC: bool, const IS_I64: bool>( // 30. Else if timeoutTime is finite, then // a. Perform EnqueueAtomicsWaitAsyncTimeoutJob(WL, waiterRecord). - let data_block_clone = buffer.get_data_block(agent).clone(); + let data_block_clone = data_block.clone(); enqueue_atomics_wait_async_job::( agent, data_block_clone, byte_index_in_buffer, + waiter_record.clone(), t, promise, gc, ); + // 31. Perform LeaveCriticalSection(WL). + drop(guard); + // 33. Perform ! CreateDataPropertyOrThrow(resultObject, "async", true). // 34. Perform ! CreateDataPropertyOrThrow(resultObject, "value", promiseCapability.[[Promise]]). let result_object = @@ -1678,6 +1681,7 @@ fn enqueue_atomics_wait_async_job( agent: &mut Agent, data_block: SharedDataBlock, byte_index_in_buffer: usize, + waiter_record: Arc, t: u64, promise: Global, gc: NoGcScope, @@ -1685,18 +1689,11 @@ fn enqueue_atomics_wait_async_job( // 1. Let timeoutJob be a new Job Abstract Closure with no parameters that // captures WL and waiterRecord and performs the following steps when // called: - let signal = Arc::new(AtomicBool::new(false)); - let s = signal.clone(); let handle = thread::spawn(move || { // SAFETY: buffer is a cloned SharedDataBlock; non-dangling. let waiters = unsafe { data_block.get_or_init_waiters() }; - let waiter_record = WaiterRecord::new_shared(); let mut guard = waiters.lock().unwrap(); - // Signal the main thread that we have the lock and are about to sleep. - s.store(true, StdOrdering::Release); - guard.push_to_list(byte_index_in_buffer, waiter_record.clone()); - if t == u64::MAX { waiter_record.wait(guard); } else { @@ -1707,6 +1704,8 @@ fn enqueue_atomics_wait_async_job( guard.remove_from_list(byte_index_in_buffer, waiter_record); // 31. Perform LeaveCriticalSection(WL). + drop(guard); + // 32. If mode is sync, return waiterRecord.[[Result]]. return WaitResult::TimedOut; } @@ -1721,9 +1720,6 @@ fn enqueue_atomics_wait_async_job( _has_timeout: t != u64::MAX, }))), }; - while !signal.load(StdOrdering::Acquire) { - // Wait until the thread has started up and is about to go to sleep. - } // 2. Let now be the time value (UTC) identifying the current time. // 3. Let currentRealm be the current Realm Record. // 4. Perform HostEnqueueTimeoutJob(timeoutJob, currentRealm, 𝔽(waiterRecord.[[TimeoutTime]]) - now). From d83e06aa1b939b479907a27927436ae66ebcaef3 Mon Sep 17 00:00:00 2001 From: Felipe Armoni Date: Thu, 16 Apr 2026 20:04:52 -0300 Subject: [PATCH 5/5] Linter fixes --- .../fundamental_objects/object_objects/object_constructor.rs | 2 +- nova_vm/src/ecmascript/builtins/structured_data/json_object.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs b/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs index 234e8e936..9bb7f74a7 100644 --- a/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs +++ b/nova_vm/src/ecmascript/builtins/fundamental_objects/object_objects/object_constructor.rs @@ -1244,7 +1244,7 @@ fn object_define_properties<'gc>( descriptors.push(Some(desc)); } // 5. For each element property of descriptors, do - for (property_key, property_descriptor) in keys.iter(agent).zip(descriptors.into_iter()) { + for (property_key, property_descriptor) in keys.iter(agent).zip(descriptors) { let Some(property_descriptor) = property_descriptor else { continue; }; diff --git a/nova_vm/src/ecmascript/builtins/structured_data/json_object.rs b/nova_vm/src/ecmascript/builtins/structured_data/json_object.rs index 1d707972d..4819275b3 100644 --- a/nova_vm/src/ecmascript/builtins/structured_data/json_object.rs +++ b/nova_vm/src/ecmascript/builtins/structured_data/json_object.rs @@ -355,7 +355,7 @@ impl JSONObject { // b. Set spaceMV to min(10, spaceMV). // c. If spaceMV < 1, let gap be the empty String; otherwise let gap be the String value containing spaceMV occurrences of the code unit 0x0020 (SPACE). let space_mv = space_mv.into_i64().clamp(0, 10) as usize; - " ".repeat(space_mv as usize).into() + " ".repeat(space_mv).into() } else if let Ok(space) = String::try_from(space) { // 8. Else if space is a String, then let space = space.to_string_lossy_(agent);