-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): root event/listener dispatch copies across moving GC #10606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| ### Fixed | ||
|
|
||
| Event/listener dispatch loops in several runtime and stdlib sites cloned | ||
| listener callbacks and call arguments into plain Rust locals, then called | ||
| into user code that could allocate and trigger a moving minor collection, | ||
| then reused those unrooted copies for the next listener. A `class X extends | ||
| EventEmitter` with an allocation-heavy listener could segfault dereferencing | ||
| a later listener's stale closure pointer — reproducibly, on a plain default | ||
| build with no GC environment knobs. `PERRY_GC_DIAG=1 | ||
| PERRY_GC_PROTECT_FROMSPACE=1` isolated the fault to a retired-from-space | ||
| dereference of a `GC_TYPE_CLOSURE` object, and the crash disappeared under | ||
| `PERRY_GEN_GC=0` (full mark-sweep, non-moving), confirming the moving | ||
| collector as the cause. | ||
|
|
||
| The primary site is `crates/perry-runtime/src/node_stream_event_emitter.rs` | ||
| (`emit_stream_event`/`call_listener_args`) — the path every `class X extends | ||
| EventEmitter` subclass and every Node stream class (`Readable`, `Writable`, | ||
| `Duplex`, `Transform`) actually dispatches through. The same pattern is also | ||
| fixed in `perry-stdlib`'s `events.rs` (`js_event_emitter_emit`/`emit0`, | ||
| `dispatch_error_monitor`, `emit_meta_event`), `domain.rs` | ||
| (`emit_domain_event`), `worker_threads/worker_surface.rs` | ||
| (`stream_emit_event`), and `events/warnings.rs` (`emit_warning`). Every | ||
| listener snapshot and call-argument copy that must stay live across a | ||
| dispatch loop is now rooted through a `RuntimeHandleScope`, re-reading each | ||
| value's current (possibly relocated) address before every call instead of | ||
| trusting the pre-call copy. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -270,9 +270,24 @@ unsafe fn emit_domain_event(handle: Handle, event: &str, args: &[f64]) -> bool { | |
| return false; | ||
| } | ||
| let receiver = nanbox_handle(handle); | ||
| for listener in listeners { | ||
| // #10600: `listeners` and `args` are plain Rust locals cloned out of the | ||
| // live domain, not GC roots. A listener can allocate enough to trigger a | ||
| // moving minor collection; an unrooted copy then holds a retired | ||
| // from-space address for the NEXT listener in this same loop. Root both | ||
| // through one handle scope and re-read the current bits before every | ||
| // call. | ||
| let scope = perry_runtime::gc::RuntimeHandleScope::new(); | ||
| let listener_handles = scope.root_nanbox_f64_slice(&listeners); | ||
| let arg_handles = scope.root_nanbox_f64_slice(args); | ||
| for listener_handle in &listener_handles { | ||
| let live_args = | ||
| perry_runtime::gc::RuntimeHandleScope::refreshed_nanbox_f64_slice(&arg_handles); | ||
| let previous_this = perry_runtime::object::js_implicit_this_set(receiver); | ||
| let _ = perry_runtime::closure::js_native_call_value(listener, args.as_ptr(), args.len()); | ||
| let _ = perry_runtime::closure::js_native_call_value( | ||
| listener_handle.get_nanbox_f64(), | ||
| live_args.as_ptr(), | ||
| live_args.len(), | ||
| ); | ||
|
Comment on lines
+286
to
+290
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win Root
Store Proposed fix- let previous_this = perry_runtime::object::js_implicit_this_set(receiver);
+ let previous_this = scope.root_nanbox_f64(
+ perry_runtime::object::js_implicit_this_set(receiver),
+ );
let _ = perry_runtime::closure::js_native_call_value(
listener_handle.get_nanbox_f64(),
live_args.as_ptr(),
live_args.len(),
);
- perry_runtime::object::js_implicit_this_set(previous_this);
+ perry_runtime::object::js_implicit_this_set(previous_this.get_nanbox_f64());🤖 Prompt for AI Agents |
||
| perry_runtime::object::js_implicit_this_set(previous_this); | ||
| } | ||
| true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -261,6 +261,27 @@ struct Listener { | |
| once: bool, | ||
| } | ||
|
|
||
| /// #10600: `Listener.callback` is a raw, untagged heap pointer to a closure. | ||
| /// `snapshot` (cloned out of the live event map before dispatch, so a | ||
| /// once-listener removal mid-dispatch doesn't affect the emit already in | ||
| /// progress) is a plain Rust `Vec`, not a GC root. A listener can allocate | ||
| /// enough to trigger a moving minor collection; an unrooted `snapshot` entry | ||
| /// then holds a retired from-space address for the NEXT listener in the same | ||
| /// dispatch loop. Root every live callback through `scope` up front and read | ||
| /// each one back through its handle — never through `snapshot` itself — at | ||
| /// call time. | ||
| fn root_listener_callbacks<'scope>( | ||
| scope: &'scope perry_runtime::gc::RuntimeHandleScope, | ||
| snapshot: &[Listener], | ||
| ) -> Vec<perry_runtime::gc::RuntimeHandle<'scope>> { | ||
| let raw: Vec<u64> = snapshot | ||
| .iter() | ||
| .filter(|l| l.callback != 0) | ||
| .map(|l| l.callback as u64) | ||
| .collect(); | ||
| scope.root_heap_word_u64_slice(&raw) | ||
| } | ||
|
|
||
| #[derive(Copy, Clone)] | ||
| struct PendingOnce { | ||
| promise: *mut Promise, | ||
|
|
@@ -406,11 +427,17 @@ impl EventEmitterHandle { | |
| let str_ptr = js_string_from_bytes(bytes.as_ptr(), bytes.len() as u32); | ||
| let event_arg = js_nanbox_string(str_ptr as i64); | ||
| let listener_arg = js_nanbox_pointer(listener_arg); | ||
| for l in snapshot { | ||
| if l.callback != 0 { | ||
| let closure_ptr = l.callback as *const ClosureHeader; | ||
| js_closure_call2(closure_ptr, event_arg, listener_arg); | ||
| } | ||
| let scope = perry_runtime::gc::RuntimeHandleScope::new(); | ||
| let event_arg_h = scope.root_nanbox_f64(event_arg); | ||
| let listener_arg_h = scope.root_nanbox_f64(listener_arg); | ||
| let callback_handles = root_listener_callbacks(&scope, &snapshot); | ||
|
Comment on lines
+430
to
+433
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift 🔎 Supported by static analysis🏁 Script executed: sed -n '255,300p;400,460p;1060,1200p' crates/perry-stdlib/src/events.rsRepository: PerryTS/perry Length of output: 9931 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- dispatch_error_monitor and related calls ---'
rg -n -A45 -B12 'fn dispatch_error_monitor|dispatch_error_monitor\(' crates/perry-stdlib/src/events.rs
printf '%s\n' '--- root handle definitions ---'
rg -n -A90 -B15 'struct RuntimeHandleScope|impl RuntimeHandleScope|root_heap_word_u64_slice|root_nanbox_f64' crates/perry-runtime crates/perry-stdlib
printf '%s\n' '--- js_string_from_bytes binding ---'
rg -n -A35 -B12 'fn js_string_from_bytes|js_string_from_bytes' crates/perry-runtime crates/perry-stdlibRepository: PerryTS/perry Length of output: 50370 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- events.rs dispatch helper ---'
rg -n -A55 -B10 '^fn dispatch_error_monitor|dispatch_error_monitor\(' crates/perry-stdlib/src/events.rs
printf '%s\n' '--- RuntimeHandleScope declaration files ---'
rg -l 'pub struct RuntimeHandleScope|struct RuntimeHandleScope' crates/perry-runtime crates/perry-stdlib | head -20
printf '%s\n' '--- handle implementation ---'
for f in $(rg -l 'pub struct RuntimeHandleScope|struct RuntimeHandleScope' crates/perry-runtime crates/perry-stdlib | head -5); do
rg -n -A120 -B10 'pub struct RuntimeHandleScope|struct RuntimeHandleScope|root_heap_word_u64_slice|root_nanbox_f64' "$f"
done
printf '%s\n' '--- string allocator definition ---'
rg -n -A35 -B10 'pub extern .*js_string_from_bytes|fn js_string_from_bytes' crates/perry-runtime/srcRepository: PerryTS/perry Length of output: 37747 Root listener callbacks before allocation and error-monitor callbacks. In On the Create the scope and root 🤖 Prompt for AI Agents |
||
| for handle in &callback_handles { | ||
| let closure_ptr = handle.get_heap_word_u64() as *const ClosureHeader; | ||
| js_closure_call2( | ||
| closure_ptr, | ||
| event_arg_h.get_nanbox_f64(), | ||
| listener_arg_h.get_nanbox_f64(), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -778,14 +805,15 @@ unsafe fn dispatch_error_monitor(emitter: &mut EventEmitterHandle, arg: Option<f | |
| emitter.prune_event_if_empty(ERROR_MONITOR_EVENT_NAME); | ||
| } | ||
|
|
||
| for l in snapshot { | ||
| if l.callback != 0 { | ||
| let closure_ptr = l.callback as *const ClosureHeader; | ||
| if let Some(arg) = arg { | ||
| js_closure_call1(closure_ptr, arg); | ||
| } else { | ||
| js_closure_call0(closure_ptr); | ||
| } | ||
| let scope = perry_runtime::gc::RuntimeHandleScope::new(); | ||
| let arg_handle = arg.map(|a| scope.root_nanbox_f64(a)); | ||
| let callback_handles = root_listener_callbacks(&scope, &snapshot); | ||
| for handle in &callback_handles { | ||
| let closure_ptr = handle.get_heap_word_u64() as *const ClosureHeader; | ||
| if let Some(arg_handle) = &arg_handle { | ||
| js_closure_call1(closure_ptr, arg_handle.get_nanbox_f64()); | ||
| } else { | ||
| js_closure_call0(closure_ptr); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1070,13 +1098,20 @@ pub unsafe extern "C" fn js_event_emitter_emit( | |
|
|
||
| let capture_rejections = emitter.capture_rejections && event_name != "error"; | ||
| let async_handle = emitter.async_resource_handle; | ||
| for l in snapshot { | ||
| if l.callback != 0 { | ||
| let result = | ||
| call_emitter_listener(handle, async_handle, l.callback, &emitted_args); | ||
| if capture_rejections { | ||
| capture_listener_rejection(handle, result); | ||
| } | ||
| let scope = perry_runtime::gc::RuntimeHandleScope::new(); | ||
| let arg_handles = scope.root_nanbox_f64_slice(&emitted_args); | ||
| let callback_handles = root_listener_callbacks(&scope, &snapshot); | ||
| for handle_cb in &callback_handles { | ||
| let live_args = | ||
| perry_runtime::gc::RuntimeHandleScope::refreshed_nanbox_f64_slice(&arg_handles); | ||
| let result = call_emitter_listener( | ||
| handle, | ||
| async_handle, | ||
| handle_cb.get_heap_word_u64() as i64, | ||
| &live_args, | ||
| ); | ||
| if capture_rejections { | ||
| capture_listener_rejection(handle, result); | ||
| } | ||
| } | ||
| } | ||
|
|
@@ -1143,12 +1178,17 @@ pub unsafe extern "C" fn js_event_emitter_emit0(handle: Handle, event_bits: i64) | |
|
|
||
| let capture_rejections = emitter.capture_rejections && event_name != "error"; | ||
| let async_handle = emitter.async_resource_handle; | ||
| for l in snapshot { | ||
| if l.callback != 0 { | ||
| let result = call_emitter_listener(handle, async_handle, l.callback, &[]); | ||
| if capture_rejections { | ||
| capture_listener_rejection(handle, result); | ||
| } | ||
| let scope = perry_runtime::gc::RuntimeHandleScope::new(); | ||
| let callback_handles = root_listener_callbacks(&scope, &snapshot); | ||
| for handle_cb in &callback_handles { | ||
| let result = call_emitter_listener( | ||
| handle, | ||
| async_handle, | ||
| handle_cb.get_heap_word_u64() as i64, | ||
| &[], | ||
| ); | ||
| if capture_rejections { | ||
| capture_listener_rejection(handle, result); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,9 +48,24 @@ unsafe fn emit_warning(warning: f64) { | |
| let key_ptr = js_string_from_bytes(key.as_ptr(), key.len() as u32); | ||
| let emit_warning = js_object_get_field_by_name_f64(process_obj, key_ptr); | ||
| if closure_ptr_from_value(emit_warning).is_some() { | ||
| let args = [warning]; | ||
| let previous_this = perry_runtime::object::js_implicit_this_set(process); | ||
| perry_runtime::closure::js_native_call_value(emit_warning, args.as_ptr(), args.len()); | ||
| // #10600: `emit_warning`, `process` and `warning` are plain Rust | ||
| // locals, not GC roots. `js_native_call_value` can itself | ||
| // allocate in its own dispatch preamble before it reads the | ||
| // callee/args, which would otherwise leave these pointing at a | ||
| // retired from-space address by the time they're dereferenced. | ||
| let scope = perry_runtime::gc::RuntimeHandleScope::new(); | ||
| let callback_h = scope.root_nanbox_f64(emit_warning); | ||
| let process_h = scope.root_nanbox_f64(process); | ||
| let arg_handles = scope.root_nanbox_f64_slice(&[warning]); | ||
| let previous_this = | ||
| perry_runtime::object::js_implicit_this_set(process_h.get_nanbox_f64()); | ||
|
Comment on lines
+60
to
+61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- affected symbols and context ---'
rg -n -C 18 'js_implicit_this_set|previous_this|prev_this' crates/perry-stdlib/src/events/warnings.rs crates/perry-stdlib/src/worker_threads/worker_surface.rs crates/perry-runtime
printf '%s\n' '--- runtime rooting APIs and uses ---'
rg -n -C 5 'root|Root|gc.*root|nanbox_f64|js_implicit_this' crates/perry-runtime crates/perry-stdlib/src | head -n 500Repository: PerryTS/perry Length of output: 50370 🤖 get_repo_knowledge executed:
Length of output: 29275 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- warnings callback path ---'
sed -n '1,105p' crates/perry-stdlib/src/events/warnings.rs
printf '%s\n' '--- worker callback path ---'
sed -n '360,430p' crates/perry-stdlib/src/worker_threads/worker_surface.rs
printf '%s\n' '--- bound declarations ---'
rg -n -C 12 'fn js_implicit_this_set|pub .*js_implicit_this_set|struct RuntimeHandleScope|impl RuntimeHandleScope|root_nanbox_(f64|u64)' crates/perry-runtime/src crates/perry-stdlib/src | head -n 300Repository: PerryTS/perry Length of output: 33962 Root the saved implicit receiver across each callback. In both 📍 Affects 2 files
🤖 Prompt for AI AgentsSource: Learnings |
||
| let live_args = | ||
| perry_runtime::gc::RuntimeHandleScope::refreshed_nanbox_f64_slice(&arg_handles); | ||
| perry_runtime::closure::js_native_call_value( | ||
| callback_h.get_nanbox_f64(), | ||
| live_args.as_ptr(), | ||
| live_args.len(), | ||
| ); | ||
| perry_runtime::object::js_implicit_this_set(previous_this); | ||
| return; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -387,12 +387,22 @@ fn stream_emit_event(event: f64, arg: f64) -> f64 { | |||||||||||||||||||||||||||
| let Some(arr) = array_ptr_from_value(get_object_field_from_value(this, &key)) else { | ||||||||||||||||||||||||||||
| return js_bool(false); | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| let args = [arg]; | ||||||||||||||||||||||||||||
| let len = perry_runtime::array::js_array_length(arr); | ||||||||||||||||||||||||||||
| // #10600: `this`, `arr` and `arg` are plain Rust locals, not GC roots. | ||||||||||||||||||||||||||||
| // `arr` is the listener array's own raw pointer, so a stale copy after a | ||||||||||||||||||||||||||||
| // listener allocates enough to trigger a moving minor collection | ||||||||||||||||||||||||||||
| // corrupts every read for the rest of this loop, not just one listener. | ||||||||||||||||||||||||||||
| // Root all three through one handle scope and re-read the current bits | ||||||||||||||||||||||||||||
| // before every dispatch. | ||||||||||||||||||||||||||||
| let scope = perry_runtime::gc::RuntimeHandleScope::new(); | ||||||||||||||||||||||||||||
| let this_h = scope.root_nanbox_f64(this); | ||||||||||||||||||||||||||||
| let arr_h = scope.root_raw_mut_ptr(arr); | ||||||||||||||||||||||||||||
| let arg_h = scope.root_nanbox_f64(arg); | ||||||||||||||||||||||||||||
| let len = perry_runtime::array::js_array_length(arr_h.get_raw_mut_ptr()); | ||||||||||||||||||||||||||||
| for i in 0..len { | ||||||||||||||||||||||||||||
| let callback = perry_runtime::array::js_array_get_f64(arr, i); | ||||||||||||||||||||||||||||
| let prev_this = perry_runtime::object::js_implicit_this_set(this); | ||||||||||||||||||||||||||||
| let callback = perry_runtime::array::js_array_get_f64(arr_h.get_raw_mut_ptr(), i); | ||||||||||||||||||||||||||||
| let prev_this = perry_runtime::object::js_implicit_this_set(this_h.get_nanbox_f64()); | ||||||||||||||||||||||||||||
| unsafe { | ||||||||||||||||||||||||||||
| let args = [arg_h.get_nanbox_f64()]; | ||||||||||||||||||||||||||||
| let _ = | ||||||||||||||||||||||||||||
| perry_runtime::closure::js_native_call_value(callback, args.as_ptr(), args.len()); | ||||||||||||||||||||||||||||
|
Comment on lines
+402
to
407
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win 🔎 Supported by static analysis🏁 Script executed: sed -n '380,415p' crates/perry-stdlib/src/worker_threads/worker_surface.rs
rg -n 'fn js_native_call_value' -A 60 crates/perry-runtime/src
sed -n '45,110p' crates/perry-runtime/src/array/buffer_receiver.rsRepository: PerryTS/perry Length of output: 14537 Root each callback before native dispatch.
The repository's rooting contract requires this. The comparable dispatch site Root Proposed fix for i in 0..len {
let callback = perry_runtime::array::js_array_get_f64(arr_h.get_raw_mut_ptr(), i);
+ let callback_h = scope.root_nanbox_f64(callback);
let prev_this = perry_runtime::object::js_implicit_this_set(this_h.get_nanbox_f64());
unsafe {
let args = [arg_h.get_nanbox_f64()];
let _ =
- perry_runtime::closure::js_native_call_value(callback, args.as_ptr(), args.len());
+ perry_runtime::closure::js_native_call_value(callback_h.get_nanbox_f64(), args.as_ptr(), args.len());
}
perry_runtime::object::js_implicit_this_set(prev_this);
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import { EventEmitter } from "node:events"; | ||
|
|
||
| // #10600: `class X extends EventEmitter` dispatches through | ||
| // perry-runtime's node_stream_event_emitter.rs, whose emit loop cloned | ||
| // listener callbacks (and the call args) into plain Rust locals, then | ||
| // called into user code that can allocate, then reused those unrooted | ||
| // copies for the next listener. A moving minor collection triggered by | ||
| // an allocation-heavy listener left the NEXT listener's snapshot entry | ||
| // pointing at retired from-space memory — no GC env knobs needed, this | ||
| // crashes under the plain default build. | ||
| class Bus extends EventEmitter {} | ||
|
|
||
| const bus = new Bus(); | ||
| const seen: string[] = []; | ||
|
|
||
| bus.on("tick", function (this: any, tag: string) { | ||
| seen.push("first:" + tag); | ||
| }); | ||
|
|
||
| bus.on("tick", function (this: any, tag: string) { | ||
| // Allocate enough that a moving minor collection lands here, while the | ||
| // THIRD listener's closure is still a live, unrooted pointer captured | ||
| // by the emit loop's listener snapshot. | ||
| const churn: any[] = []; | ||
| for (let i = 0; i < 6000; i++) { | ||
| churn.push({ k: i, s: "x" + i, o: { i, s2: "y" + i } }); | ||
| } | ||
| seen.push("second:" + tag + ":" + churn.length); | ||
| }); | ||
|
|
||
| bus.on("tick", function (this: any, tag: string) { | ||
| seen.push("third:" + tag); | ||
| }); | ||
|
|
||
| for (let i = 0; i < 300; i++) { | ||
| bus.emit("tick", "t" + i); | ||
| } | ||
|
|
||
| console.log(seen.length, seen[seen.length - 1]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 8493
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 20726
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 11908
Root listener snapshots before removing once listeners and use refreshed handles.
remove_once_listenersemitsremoveListenerthroughemit_meta_event. When that event has listeners,emit_meta_eventdispatches user JavaScript throughemit_stream_eventandjs_native_call_value. The callback can allocate and move values inmonitor_snapshotorsnapshotbefore their handles are created. The later loops can then call stale callback bits.The function also reads the unrooted
stream,event, andargslocals after creating handles. Usestream_h,event_h, and refreshedarg_handlesfor every post-scope read.Create
monitor_handlesandlistener_handlesbefore their respectiveremove_once_listenerscalls. Pass each callback through its handle when dispatching.🤖 Prompt for AI Agents