diff --git a/changelog.d/8177-handle-bound-method-name-static.md b/changelog.d/8177-handle-bound-method-name-static.md new file mode 100644 index 0000000000..79547b2946 --- /dev/null +++ b/changelog.d/8177-handle-bound-method-name-static.md @@ -0,0 +1,75 @@ +Fixed six runtime sites that bound a **movable GC heap string's interior** as a +bound-method name (#8133), on the timer-handle and TextDecoder/TextEncoder +property paths. + +`js_class_method_bind(instance, name_ptr, name_len)` stores the name POINTER in +the bound closure and `dispatch_bound_method` re-reads it at CALL time, so the +pointer must outlive the closure — a contract codegen satisfies with per-module +rodata. Each of these callers instead derived +`key_ptr = (key as *const u8).add(size_of::())`, the interior of a +heap string that is unreachable the moment the read returns, so a copying minor +could relocate or reclaim the bytes the closure names. #7747 fixed the identical +defect on the Buffer path; its commit message states the consequence — "whether +the stale bytes still spell the method is an allocator property, not a program +property" — which is why that one passed locally and took a SIGSEGV on +conformance-smoke. + +Four sites are the ones #8133 names: the timer-handle arm in +`get_field_by_name_tail.rs` twice (NaN-boxed small-handle and already-stripped +handle-band receivers), and `text.rs`'s `text_handle_property` for +`TextDecoder.prototype.decode` and `TextEncoder.prototype.encode`/`encodeInto`. +Two more of the identical timer block were found while confirming those: +`ic_miss.rs`'s inline-cache MISS mirror (a separate LIVE entry point — its own +comment says the IC fast path funnels small handles there, bypassing the block in +`js_object_get_field_by_name`) and a third copy in `get_field_by_name.rs` that +looks shadowed by the tail today and is fixed defensively. + +`is_timer_handle_method_key` is REPLACED by +`timer_handle_method_name_static(key) -> Option<&'static [u8]>`, and `text.rs` +grows `text_decoder_method_name_static` / `text_encoder_method_name_static`. +Answering the literal instead of a `bool` is the fix, not a refactor: with no +predicate left, a caller has nothing to pair with its own pointer, so writing the +obvious code cannot reintroduce the bug. `text_handle_property` goes one further +and no longer TAKES `key_ptr`/`key_len` at all — it cannot bind the caller's +pointer because it no longer has it. + +**This one reproduces.** A literal `dec.decode` lowers the name to rodata and +never reaches these arms; a COMPUTED key does not. On a pre-fix binary, +`const k = "dec" + "ode"; const f = (dec as any)[k];` followed by 400k +allocations prints `decode=undefined` and then throws where node prints +`decode=hi` — a silent wrong answer with no instruments at all — and under +`PERRY_GC_ZEAL=1 PERRY_GC_ZEAL_ALLOC_KB=0 PERRY_GC_PROTECT_FROMSPACE=1 +PERRY_GC_PROTECT_FROMSPACE_DEPTH=800` it takes a SIGBUS the protector reports as +`RETIRED FROM-SPACE … retired_by_minor=#0 … obj_type=3`. Fixed, the same fixture +matches node byte-for-byte and exits 0 with the protector ARMED +(`retired_set=#0 blocks=18 bytes_protected=18874368`), so the green run means the +detector was live rather than that nothing was tried. + +Six tests in `gc/tests/handle_bound_method_name.rs`, mirroring +`buffer_bound_method_name.rs`: they assert POINTER IDENTITY with the `'static` +literal, because — per #7747's note, which #8133 repeats — an inequality against +the key could pass with the bug present, and comparing the BYTES only fails on a +host where the freed memory has already been reused. Each also asserts its gate +is live (`is_known_timer_id` / `is_known_text_decoder_id`) before measuring, so a +green run cannot mean the arm never ran. Two sabotage arms were run and reverted: +the timer lookup echoing its argument (restoring the exact pre-fix pointer at all +four timer sites) fails the three timer tests plus the no-borrow test, and the +text lookups echoing theirs fails the two text tests. + +One measurement of mine was vacuous before it was fixed: the tests initially +failed WITH the fix applied, because the helper compared against a `b"ref"` +literal written in the test file — two occurrences of the same byte string in +different modules are two `&'static [u8]`s the linker may leave at different +addresses, and it did. The expected pointer now comes from the lookup under test, +which is what `buffer_bound_method_name.rs` does and why it was right. + +Two related surfaces are deliberately left for their own issue: the +primitive-receiver arm in `get_field_by_name.rs` (`(5).toString` &c.), and +`perry-stdlib`'s handle-property dispatch layer, where +`js_handle_property_dispatch` forwards the same pointer and eight sub-dispatchers +(`sqlite/dispatch.rs` ×4, `tls/dispatch.rs` ×2, +`common/dispatch/emitter_als.rs` ×2) capture it directly rather than remapping to +a literal — so `const f = db.run` / `emitter.on` / `als.getStore` carry the same +hazard. + +`cargo test -p perry-runtime --lib`: 2424 passed, 0 failed, 4 ignored. diff --git a/crates/perry-runtime/src/gc/tests/handle_bound_method_name.rs b/crates/perry-runtime/src/gc/tests/handle_bound_method_name.rs new file mode 100644 index 0000000000..e2684883c0 --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/handle_bound_method_name.rs @@ -0,0 +1,273 @@ +//! A bound TIMER-handle / TextDecoder / TextEncoder method closure must not +//! capture a pointer into the key string. +//! +//! `js_class_method_bind` stores the method-name POINTER in the closure +//! (capture 1) and `dispatch_bound_method` re-reads it at CALL time. Its +//! contract says so: "Method-name pointer is expected to be stable for the +//! closure's lifetime; codegen emits it from the per-module `.str.N.bytes` +//! rodata global." +//! +//! #7747 fixed two Buffer callers that broke it (see +//! `buffer_bound_method_name.rs`). Its commit message states the failure mode +//! exactly, and it applies verbatim here: +//! +//! > `get_field_by_name_tail` passed `key + size_of::()` — the +//! > interior of a movable GC heap string that is unreachable once the read +//! > returns — so a copying minor could relocate or reclaim the bytes the +//! > closure names. […] Whether the stale bytes still spell the method is an +//! > allocator property, not a program property, which is why this passed +//! > locally and took a SIGSEGV on conformance-smoke shards 7 and 8. +//! +//! #8133 is the same defect at four more sites in the same neighbourhood, plus +//! two the issue did not name: +//! +//! * `get_field_by_name_tail.rs` — the timer-handle arm, twice (a NaN-boxed +//! small-handle receiver and an already-stripped handle-band one). +//! * `text.rs`'s `text_handle_property` — `TextDecoder.prototype.decode` and +//! `TextEncoder.prototype.encode`/`encodeInto` read as VALUES. That +//! function's own docstring says value reads are the reason it exists +//! (`K.decode.bind(K)`, "the shape a minified SDK's cached decodeText helper +//! takes"), so this is the intended hot path, not an edge. +//! * `ic_miss.rs` — the inline-cache MISS mirror, whose own comment says "the +//! IC fast path funnels small handles here, bypassing the identical block in +//! `js_object_get_field_by_name`, so it must be mirrored". A separate live +//! entry point, not a redundant copy. +//! * `get_field_by_name.rs` — a third copy of the same timer block. Fixed +//! defensively; nothing guarantees it stays unreachable across refactors. +//! +//! ## Why these assert IDENTITY and not bytes +//! +//! Quoting #7747's own testing note, which the issue repeats: the inequality +//! against the key string could pass with the bug present, and comparing the +//! BYTES only fails on a host where the freed memory has already been reused — +//! which is the lucky-allocator problem these tests exist to avoid. **Identity +//! with the literal cannot be lucky.** So every test below asserts +//! `captured_ptr == .as_ptr()`. +//! +//! A test that merely called the bound method after a collection would pass +//! with the bug fully present on any host whose allocator left the bytes +//! intact, which is precisely the test not to write. + +use super::support::*; + +/// The name bytes a bound closure keeps, as raw parts. Same helper shape as +/// `buffer_bound_method_name.rs`. +unsafe fn captured_name(bound: crate::value::JSValue) -> (*const u8, usize) { + let closure = crate::value::js_nanbox_get_pointer(f64::from_bits(bound.bits())) + as *const crate::ClosureHeader; + assert!(!closure.is_null(), "the read must produce a bound closure"); + let ptr = crate::closure::js_closure_get_capture_ptr(closure, 1) as *const u8; + let len = crate::closure::js_closure_get_capture_ptr(closure, 2) as usize; + (ptr, len) +} + +/// An interned heap key plus the interior pointer the buggy callers derived +/// from it (`key + size_of::()`). +unsafe fn heap_key(name: &str) -> (*mut crate::string::StringHeader, *const u8) { + let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + let interior = (key as *const u8).add(std::mem::size_of::()); + (key, interior) +} + +/// Assert the closure names the `'static` literal, and specifically NOT the +/// caller's heap-string interior. +/// +/// `expected` MUST be obtained by calling the lookup under test, never written +/// as a literal here: two occurrences of `b"ref"` in different modules are two +/// `&'static [u8]`s that the linker is free to leave at different addresses, +/// and it does. Asserting against a locally written literal fails on a correct +/// implementation — measured, before this comment existed. +unsafe fn assert_names_the_literal( + bound: crate::value::JSValue, + expected: &'static [u8], + key_interior: *const u8, + what: &str, +) { + let (name_ptr, name_len) = captured_name(bound); + assert_eq!( + name_ptr, + expected.as_ptr(), + "{what}: the closure must capture the 'static literal" + ); + assert_ne!( + name_ptr, key_interior, + "{what}: the closure captured the KEY STRING's interior — that \ + allocation is movable and unreachable after this read, so the name it \ + dispatches on is freed or relocated bytes" + ); + assert_eq!(name_len, expected.len(), "{what}: captured length"); + assert_eq!( + std::slice::from_raw_parts(name_ptr, name_len), + expected, + "{what}: captured bytes" + ); +} + +/// The `'static` literal the lookup under test answers for `key`. Never write +/// the literal locally — see [`assert_names_the_literal`]. +fn timer_literal(key: &[u8]) -> &'static [u8] { + crate::object::timer_handle_method_name_static(key).expect("a timer-handle method") +} + +/// A live `Timeout` handle id. `is_known_timer_id` gates every arm under test, +/// so without a registered timer they all decline and the tests would be +/// vacuous — `assert!(is_known_timer_id(..))` below is what says they are not. +fn live_timer() -> i64 { + let id = crate::timer::js_set_timeout_callback(0, 10_000.0); + assert!( + crate::timer::is_known_timer_id(id), + "the arms under test are gated on `is_known_timer_id`; without a live \ + timer every assertion below would pass by never running" + ); + id +} + +/// ★ The regression, NaN-boxed small-handle receiver +/// (`get_field_by_name_tail.rs`, arm 1). +#[test] +fn a_bound_timer_method_never_captures_the_key_strings_interior() { + let _guard = GcTestIsolationGuard::new(); + unsafe { + let id = live_timer(); + let (key, key_interior) = heap_key("ref"); + let boxed = crate::value::js_nanbox_pointer(id).to_bits() as *const crate::ObjectHeader; + + let bound = crate::object::js_object_get_field_by_name(boxed, key); + assert_names_the_literal(bound, timer_literal(b"ref"), key_interior, "timer.ref"); + } +} + +/// ★ The regression, already-stripped handle-band receiver +/// (`get_field_by_name_tail.rs`, arm 2). +#[test] +fn a_bound_timer_method_from_a_raw_handle_never_captures_the_key() { + let _guard = GcTestIsolationGuard::new(); + unsafe { + let id = live_timer(); + let (key, key_interior) = heap_key("unref"); + + let bound = + crate::object::js_object_get_field_by_name(id as *const crate::ObjectHeader, key); + assert_names_the_literal( + bound, + timer_literal(b"unref"), + key_interior, + "timer.unref (raw handle)", + ); + } +} + +/// ★ The regression, inline-cache MISS path (`ic_miss.rs`). A separate live +/// entry point: its own comment says the IC fast path funnels small handles +/// here, bypassing the block in `js_object_get_field_by_name`. +#[test] +fn a_bound_timer_method_from_the_ic_miss_path_never_captures_the_key() { + let _guard = GcTestIsolationGuard::new(); + unsafe { + let id = live_timer(); + let (key, key_interior) = heap_key("hasRef"); + let mut cache = crate::object::PicCache::default(); + + let bits = crate::object::js_object_get_field_ic_miss( + id as *const crate::ObjectHeader, + key, + &mut cache, + ); + assert_names_the_literal( + crate::value::JSValue::from_bits(bits.to_bits()), + timer_literal(b"hasRef"), + key_interior, + "timer.hasRef (IC miss)", + ); + } +} + +/// ★ The regression, `TextDecoder.prototype.decode` read as a VALUE — the +/// `K.decode.bind(K)` shape `text_handle_property`'s docstring exists for. +#[test] +fn a_bound_text_decoder_decode_never_captures_the_key_strings_interior() { + let _guard = GcTestIsolationGuard::new(); + unsafe { + let undefined = f64::from_bits(crate::value::TAG_UNDEFINED); + let id = crate::text::js_text_decoder_new(undefined, undefined, undefined); + assert!( + crate::text::is_known_text_decoder_id(id), + "the decoder arm is gated on `is_known_text_decoder_id`" + ); + let (key, key_interior) = heap_key("decode"); + + let bound = + crate::object::js_object_get_field_by_name(id as *const crate::ObjectHeader, key); + let expected = crate::text::text_decoder_method_name_static(b"decode") + .expect("decode is a TextDecoder method"); + assert_names_the_literal(bound, expected, key_interior, "TextDecoder.decode"); + } +} + +/// ★ The regression, `TextEncoder.prototype.encode` / `encodeInto`. +#[test] +fn a_bound_text_encoder_method_never_captures_the_key_strings_interior() { + let _guard = GcTestIsolationGuard::new(); + unsafe { + let id = crate::text::js_text_encoder_new(); + for name in ["encode", "encodeInto"] { + let (key, key_interior) = heap_key(name); + let bound = + crate::object::js_object_get_field_by_name(id as *const crate::ObjectHeader, key); + let expected = crate::text::text_encoder_method_name_static(name.as_bytes()) + .expect("a TextEncoder method"); + assert_names_the_literal(bound, expected, key_interior, name); + } + } +} + +/// The lookups must not simply echo their argument — a `|k| Some(k)` that +/// type-checked would pass every identity assertion above while still handing +/// back the caller's storage. +#[test] +fn the_static_name_lookups_do_not_borrow_their_argument() { + let _guard = GcTestIsolationGuard::new(); + + let owned = String::from("refresh"); + let found = crate::object::timer_handle_method_name_static(owned.as_bytes()) + .expect("refresh is a timer-handle method"); + assert_ne!( + found.as_ptr(), + owned.as_bytes().as_ptr(), + "the lookup must answer the LITERAL, not a borrow of its argument" + ); + + // Same literal for every caller, whatever storage the caller used. + let second = String::from("refresh"); + assert_eq!( + crate::object::timer_handle_method_name_static(second.as_bytes()) + .unwrap() + .as_ptr(), + found.as_ptr(), + "every call must answer the same 'static address" + ); + + assert!( + crate::object::timer_handle_method_name_static(b"notATimerMethod").is_none(), + "a non-method key must not resolve" + ); + // The list must not have shrunk while being rewritten: #8133 replaced the + // `is_timer_handle_method_key` predicate with this lookup, and a dropped + // name would silently stop binding that method rather than fail loudly. + for name in [ + &b"ref"[..], + b"unref", + b"hasRef", + b"refresh", + b"close", + b"__perry_dispose__", + b"@@__perry_wk_dispose", + b"@@__perry_wk_toPrimitive", + ] { + assert_eq!( + crate::object::timer_handle_method_name_static(name), + Some(name), + "every pre-#8133 timer-handle method must still resolve" + ); + } +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index 0459e9e790..382a1370dd 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -21,6 +21,7 @@ mod fromspace_protect; mod fromspace_scan; mod global_bootstrap; mod global_sink_isolation; +mod handle_bound_method_name; mod heap_accounting; mod helper_stores; mod host_safepoints; diff --git a/crates/perry-runtime/src/object/field_get_set.rs b/crates/perry-runtime/src/object/field_get_set.rs index f832cff995..060577cdcd 100644 --- a/crates/perry-runtime/src/object/field_get_set.rs +++ b/crates/perry-runtime/src/object/field_get_set.rs @@ -331,8 +331,8 @@ pub(crate) use has_property::{ }; pub use has_property::{js_in_operator, js_object_has_property}; pub(crate) use ic_miss::{ - is_array_method_value_name, is_primitive_proto_method, is_timer_handle_method_key, - set_method_value_name, + is_array_method_value_name, is_primitive_proto_method, set_method_value_name, + timer_handle_method_name_static, }; pub use ic_miss::{ js_object_get_field_by_name_f64, js_object_get_field_by_property_id_f64, diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs index 5e36a5919a..f76d0821b3 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name.rs @@ -861,19 +861,23 @@ pub extern "C" fn js_object_get_field_by_name( (key as *const u8).add(std::mem::size_of::()); let key_len = (*key).byte_len as usize; let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); - if is_timer_handle_method_key(key_bytes) - && crate::timer::is_known_timer_id(raw as i64) - { - let this_f64 = - f64::from_bits(crate::value::js_nanbox_pointer(raw as i64).to_bits()); - let result = super::super::js_class_method_bind(this_f64, key_ptr, key_len); - return JSValue::from_bits(result.to_bits()); + if let Some(method) = timer_handle_method_name_static(key_bytes) { + if crate::timer::is_known_timer_id(raw as i64) { + let this_f64 = f64::from_bits( + crate::value::js_nanbox_pointer(raw as i64).to_bits(), + ); + // #8133: the `'static` literal, NOT `key_ptr`. + let result = super::super::js_class_method_bind( + this_f64, + method.as_ptr(), + method.len(), + ); + return JSValue::from_bits(result.to_bits()); + } } // TextDecoder/TextEncoder registry handles — see // `text_handle_property` (text.rs). - if let Some(v) = - crate::text::text_handle_property(raw, key_bytes, key_ptr, key_len) - { + if let Some(v) = crate::text::text_handle_property(raw, key_bytes) { return v; } if key_bytes == b"constructor" { diff --git a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs index 312dd23dc8..3783f356ed 100644 --- a/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs +++ b/crates/perry-runtime/src/object/field_get_set/get_field_by_name_tail.rs @@ -38,22 +38,24 @@ pub(crate) fn get_field_by_name_object_tail( (key as *const u8).add(std::mem::size_of::()); let key_len = (*key).byte_len as usize; let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); - if is_timer_handle_method_key(key_bytes) - && crate::timer::is_known_timer_id(raw as i64) - { - let this_f64 = f64::from_bits( - crate::value::js_nanbox_pointer(raw as i64).to_bits(), - ); - let result = - super::super::js_class_method_bind(this_f64, key_ptr, key_len); - return JSValue::from_bits(result.to_bits()); + if let Some(method) = timer_handle_method_name_static(key_bytes) { + if crate::timer::is_known_timer_id(raw as i64) { + let this_f64 = f64::from_bits( + crate::value::js_nanbox_pointer(raw as i64).to_bits(), + ); + // #8133: the `'static` literal, NOT `key_ptr` — + // that is the interior of a movable heap string + // this read does not own. + let result = super::super::js_class_method_bind( + this_f64, + method.as_ptr(), + method.len(), + ); + return JSValue::from_bits(result.to_bits()); + } } - if let Some(v) = crate::text::text_handle_property( - raw as usize, - key_bytes, - key_ptr, - key_len, - ) { + if let Some(v) = crate::text::text_handle_property(raw as usize, key_bytes) + { return v; } } @@ -113,17 +115,20 @@ pub(crate) fn get_field_by_name_object_tail( let key_ptr = (key as *const u8).add(std::mem::size_of::()); let key_len = (*key).byte_len as usize; let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); - if is_timer_handle_method_key(key_bytes) - && crate::timer::is_known_timer_id(obj as i64) - { - let this_f64 = - f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); - let result = super::super::js_class_method_bind(this_f64, key_ptr, key_len); - return JSValue::from_bits(result.to_bits()); + if let Some(method) = timer_handle_method_name_static(key_bytes) { + if crate::timer::is_known_timer_id(obj as i64) { + let this_f64 = + f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); + // #8133: see the sibling arm above. + let result = super::super::js_class_method_bind( + this_f64, + method.as_ptr(), + method.len(), + ); + return JSValue::from_bits(result.to_bits()); + } } - if let Some(v) = - crate::text::text_handle_property(obj as usize, key_bytes, key_ptr, key_len) - { + if let Some(v) = crate::text::text_handle_property(obj as usize, key_bytes) { return v; } } diff --git a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs index 152b823aeb..bca9274a3b 100644 --- a/crates/perry-runtime/src/object/field_get_set/ic_miss.rs +++ b/crates/perry-runtime/src/object/field_get_set/ic_miss.rs @@ -169,20 +169,38 @@ pub(crate) fn set_method_value_name(key: &[u8]) -> Option<&'static [u8]> { } } -pub(crate) fn is_timer_handle_method_key(key: &[u8]) -> bool { - matches!( - key, - b"ref" - | b"unref" - | b"hasRef" - | b"refresh" - | b"close" - | b"__perry_dispose__" - // `using t = setTimeout(...)` / `t[Symbol.dispose]` — the - // well-known dispose symbol lowers to this key. (#1213) - | b"@@__perry_wk_dispose" - | b"@@__perry_wk_toPrimitive" - ) +/// A `Timeout` / `Immediate` handle method key, as a `'static` byte string: +/// the literal out of this list, never a borrow of `key`. +/// +/// #8133 — this replaced the `is_timer_handle_method_key` PREDICATE it used to +/// be, and the replacement is the fix, not a refactor. Every caller derives +/// `key` as `key_string + size_of::()` — the interior of a +/// movable GC heap string that is unreachable the moment the read returns — +/// and then handed that same pointer to `js_class_method_bind`, which captures +/// it into the bound closure for `dispatch_bound_method` to re-read at CALL +/// time. #7747 fixed exactly this on the Buffer path and its commit message +/// names the consequence: "whether the stale bytes still spell the method is an +/// allocator property, not a program property", which is why it passed locally +/// and took a SIGSEGV on conformance-smoke. +/// +/// Returning the literal rather than answering `bool` is deliberate: with no +/// predicate left, a caller has nothing to pair with its own pointer, so the +/// bug cannot be reintroduced by writing the obvious code. Same shape as +/// `set_method_value_name` above and `buffer_method_name_static` (#7747). +pub(crate) fn timer_handle_method_name_static(key: &[u8]) -> Option<&'static [u8]> { + match key { + b"ref" => Some(b"ref"), + b"unref" => Some(b"unref"), + b"hasRef" => Some(b"hasRef"), + b"refresh" => Some(b"refresh"), + b"close" => Some(b"close"), + b"__perry_dispose__" => Some(b"__perry_dispose__"), + // `using t = setTimeout(...)` / `t[Symbol.dispose]` — the well-known + // dispose symbol lowers to this key. (#1213) + b"@@__perry_wk_dispose" => Some(b"@@__perry_wk_dispose"), + b"@@__perry_wk_toPrimitive" => Some(b"@@__perry_wk_toPrimitive"), + _ => None, + } } /// Words in a per-site property-read cache global (`@perry_ic_N`). Codegen @@ -526,19 +544,24 @@ pub extern "C" fn js_object_get_field_ic_miss( let key_ptr = (key as *const u8).add(std::mem::size_of::()); let key_len = (*key).byte_len as usize; let key_bytes = std::slice::from_raw_parts(key_ptr, key_len); - if is_timer_handle_method_key(key_bytes) && crate::timer::is_known_timer_id(obj as i64) - { - let this_f64 = - f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); - return super::super::js_class_method_bind(this_f64, key_ptr, key_len); + if let Some(method) = timer_handle_method_name_static(key_bytes) { + if crate::timer::is_known_timer_id(obj as i64) { + let this_f64 = + f64::from_bits(crate::value::js_nanbox_pointer(obj as i64).to_bits()); + // #8133: the `'static` literal, NOT `key_ptr` — that is the + // interior of a movable heap string this read does not own. + return super::super::js_class_method_bind( + this_f64, + method.as_ptr(), + method.len(), + ); + } } // TextDecoder/TextEncoder registry handles — IC-miss mirror of // the arms in `js_object_get_field_by_name` / // `get_field_by_name_object_tail`; static-name reads (`td.decode`, // `td.encoding`) funnel here. See `text_handle_property`. - if let Some(v) = - crate::text::text_handle_property(obj as usize, key_bytes, key_ptr, key_len) - { + if let Some(v) = crate::text::text_handle_property(obj as usize, key_bytes) { return f64::from_bits(v.bits()); } } diff --git a/crates/perry-runtime/src/text.rs b/crates/perry-runtime/src/text.rs index d5ce51919c..cb1b098a79 100644 --- a/crates/perry-runtime/src/text.rs +++ b/crates/perry-runtime/src/text.rs @@ -636,19 +636,52 @@ static KEEP_TEXT_DECODER_IGNORE_BOM: extern "C" fn(f64) -> f64 = js_text_decoder /// "Bind must be called on a function"). Methods reify as bound methods — /// the dynamic-call arm in `native_call_method.rs` executes them on call — /// and accessors return their values directly. +/// A `TextDecoder.prototype` method key, as a `'static` byte string. +/// +/// #8133 — the bind below used to hand `js_class_method_bind` the caller's +/// `key_ptr`, which every caller derives as +/// `key_string + size_of::()`: the interior of a movable GC heap +/// string, unreachable the moment the read returns. The closure captures that +/// pointer and `dispatch_bound_method` re-reads it at CALL time, so a copying +/// minor could relocate or reclaim the bytes the closure names. #7747 fixed the +/// same defect on the Buffer path. +/// +/// `text_handle_property` no longer TAKES the caller's pointer at all, so the +/// bug is not merely fixed here, it is unwritable. +pub(crate) fn text_decoder_method_name_static(key: &[u8]) -> Option<&'static [u8]> { + match key { + b"decode" => Some(b"decode"), + _ => None, + } +} + +/// A `TextEncoder.prototype` method key, as a `'static` byte string. See +/// [`text_decoder_method_name_static`]. +pub(crate) fn text_encoder_method_name_static(key: &[u8]) -> Option<&'static [u8]> { + match key { + b"encode" => Some(b"encode"), + b"encodeInto" => Some(b"encodeInto"), + _ => None, + } +} + +/// Bind `method` — always a `'static` literal from the two lookups above — as a +/// bound-method value on `this_f64`. +fn bind_static_method(this_f64: f64, method: &'static [u8]) -> crate::value::JSValue { + let result = crate::object::js_class_method_bind(this_f64, method.as_ptr(), method.len()); + crate::value::JSValue::from_bits(result.to_bits()) +} + pub(crate) unsafe fn text_handle_property( raw: usize, key_bytes: &[u8], - key_ptr: *const u8, - key_len: usize, ) -> Option { let this_f64 = f64::from_bits(crate::value::js_nanbox_pointer(raw as i64).to_bits()); if is_known_text_decoder_id(raw as i64) { + if let Some(method) = text_decoder_method_name_static(key_bytes) { + return Some(bind_static_method(this_f64, method)); + } match key_bytes { - b"decode" => { - let result = crate::object::js_class_method_bind(this_f64, key_ptr, key_len); - return Some(crate::value::JSValue::from_bits(result.to_bits())); - } b"encoding" => { let s = js_text_decoder_encoding(this_f64); return Some(crate::value::JSValue::string_ptr(s)); @@ -666,9 +699,10 @@ pub(crate) unsafe fn text_handle_property( _ => {} } } - if raw as i64 == TEXT_ENCODER_SENTINEL_ID && matches!(key_bytes, b"encode" | b"encodeInto") { - let result = crate::object::js_class_method_bind(this_f64, key_ptr, key_len); - return Some(crate::value::JSValue::from_bits(result.to_bits())); + if raw as i64 == TEXT_ENCODER_SENTINEL_ID { + if let Some(method) = text_encoder_method_name_static(key_bytes) { + return Some(bind_static_method(this_f64, method)); + } } None }