diff --git a/changelog.d/7747-buffer-bound-method-name-lifetime.md b/changelog.d/7747-buffer-bound-method-name-lifetime.md new file mode 100644 index 0000000000..bbed19f81b --- /dev/null +++ b/changelog.d/7747-buffer-bound-method-name-lifetime.md @@ -0,0 +1,49 @@ +### Fixed + +**A bound Buffer-method closure captured a pointer to memory it did not own, +and dispatched on it later.** `typeof buf.readUInt8`, `const f = buf.readUInt8` +and `buf[k]` each produced a closure whose method-name pointer was already +dangling; the call then resolved the method from freed or relocated bytes. + +`js_class_method_bind` stores the method-name POINTER in the closure and +`dispatch_bound_method` re-reads it at CALL time. Its own doc states the +contract — *"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"* — +and codegen honours it. Two runtime callers on the Buffer path did not: + +* `get_field_by_name_tail` derived `key_ptr` as + `key + size_of::()` — the **interior of a movable GC heap + string** — and passed it through `buffer_own_prop_or_method`. The key string + is unreachable the moment the read returns, so a copying minor could relocate + or reclaim it out from under a closure that outlives it. +* `polymorphic_index`'s computed-key arm bound `name.as_bytes().as_ptr()` where + `name` is a local `String`. That one dangles on return with no collector + involvement at all. + +Both now bind a `'static` literal. `buffer_dispatch`'s method-name list becomes +a single macro-generated source for both `is_buffer_method_name` and a new +`buffer_method_name_static`, which returns the literal out of that list rather +than a borrow of its argument — so there is one list to keep current, not two. + +**Why it read as flaky.** Whether the stale bytes still spell the method name +is a property of the allocator, not of the bug, so the same code passed locally +and took a SIGSEGV on conformance-smoke: `test_gap_buffer_own_prop_shadow_intrinsic_6405` +on shard 7, joined by `test_gap_buffer_own_props` on shard 8 once collections +got denser. Neither test is in `gap_snapshot.json`; both are expected to pass. + +**Tests** — `gc/tests/buffer_bound_method_name.rs`, in the required per-PR +`cargo-test` gate, asserting the contract structurally rather than asking +whether a given run happens to survive it: + +* the closure's captured name must not alias the key string's interior; +* the computed-key arm's captured name must not come from a temporary; +* `buffer_method_name_static` must not return a borrow of its argument, and + every caller must get the same literal whatever storage its own copy lives in. + +Verified by sabotage: with the two call sites reverted and the tests unchanged, +the first two fail — the second on garbage bytes, i.e. the use-after-free +reproduced deterministically in-process on a host where the end-to-end tests +still passed. + +`cargo test -p perry-runtime --lib`: 1979 passed, 0 failed. All 12 +buffer/DataView gap tests byte-match Node 26.5.1, including both crashers. diff --git a/crates/perry-runtime/src/gc/tests/buffer_bound_method_name.rs b/crates/perry-runtime/src/gc/tests/buffer_bound_method_name.rs new file mode 100644 index 0000000000..25a8ab4fdf --- /dev/null +++ b/crates/perry-runtime/src/gc/tests/buffer_bound_method_name.rs @@ -0,0 +1,151 @@ +//! A bound Buffer-method closure must not capture a pointer into the key +//! string, or into any other storage the caller owns. +//! +//! `js_class_method_bind` stores the method-name POINTER in the closure 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." Two +//! runtime callers on the Buffer path did not honour it: +//! +//! * `get_field_by_name_tail` derived `key_ptr` as `key + size_of::()` +//! — the INTERIOR of a movable GC heap string — and passed it straight +//! through `buffer_own_prop_or_method`. Every `typeof buf.readUInt8` / +//! `const f = buf.readUInt8` read produced a closure pointing into the +//! nursery. Once that string moved under a copying minor (or was reclaimed, +//! the string being unreachable after the read), the closure named freed or +//! relocated bytes and the call dispatched on garbage. +//! * `polymorphic_index`'s computed-key arm (`buf[k]`) bound +//! `name.as_bytes().as_ptr()` where `name` is a local `String` — freed on +//! return, so the closure dangled before any collection was involved. +//! +//! The failure is invisible on a host whose allocator happens to leave the old +//! bytes intact, which is why it surfaced as a conformance-smoke SIGSEGV on +//! Linux while the same tests passed locally. So the assertions here are +//! STRUCTURAL — the captured pointer must not alias the key string at all — +//! rather than "does it happen to still read correctly after a collection", +//! which is exactly the question a lucky allocator answers wrong. + +use super::super::*; +use super::support::*; + +/// The name bytes a bound closure keeps, as raw parts. +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) +} + +/// ★ The regression. Reading a Buffer method as a VALUE must not hand the +/// closure the key string's interior. +#[test] +fn a_bound_buffer_method_never_captures_the_key_strings_interior() { + let _guard = GcTestIsolationGuard::new(); + + unsafe { + let buf = crate::buffer::buffer_alloc(8); + let key = crate::string::js_string_from_bytes(b"readUInt8".as_ptr(), 9); + let key_interior = (key as *const u8).add(std::mem::size_of::()); + + let bound = crate::object::js_object_get_field_by_name(buf as *const crate::ObjectHeader, key); + let (name_ptr, name_len) = captured_name(bound); + + let static_name = crate::object::buffer_method_name_static("readUInt8") + .expect("readUInt8 is a Buffer method"); + assert_eq!( + name_ptr, + static_name.as_ptr(), + "the closure must capture the 'static literal" + ); + assert_ne!( + name_ptr, key_interior, + "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, 9, "the captured name must still be `readUInt8`"); + assert_eq!( + std::slice::from_raw_parts(name_ptr, name_len), + b"readUInt8", + "and it must spell the method" + ); + } +} + +/// The same contract for the computed-key arm (`buf[k]`), whose name came from +/// a local `String` — dangling on return with no collection required. +#[test] +fn a_computed_key_buffer_method_never_captures_a_temporary() { + let _guard = GcTestIsolationGuard::new(); + + unsafe { + let buf = crate::buffer::buffer_alloc(8); + let key = crate::string::js_string_from_bytes(b"readUInt8".as_ptr(), 9); + let key_interior = (key as *const u8).add(std::mem::size_of::()); + let key_value = f64::from_bits(crate::value::js_nanbox_string(key as i64).to_bits()); + + let obj_handle = crate::value::js_nanbox_pointer(buf as i64).to_bits() as i64; + let bound = crate::value::JSValue::from_bits( + crate::object::js_object_get_index_polymorphic(obj_handle, key_value).to_bits(), + ); + let (name_ptr, name_len) = captured_name(bound); + + // Pointer IDENTITY with the static literal, not merely "not the key + // string". The broken version of this path captured a local `String`'s + // bytes, which are neither the key's interior nor the literal — so an + // inequality against the key would pass with the bug fully present, and + // comparing the BYTES only fails on a host where the freed memory has + // already been reused. Identity is the assertion that cannot be lucky. + let static_name = crate::object::buffer_method_name_static("readUInt8") + .expect("readUInt8 is a Buffer method"); + assert_eq!( + name_ptr, + static_name.as_ptr(), + "the computed-key arm must capture the 'static literal — anything \ + else is storage the caller owns and the closure outlives" + ); + assert_ne!( + name_ptr, key_interior, + "and in particular not the key string's interior" + ); + assert_eq!( + std::slice::from_raw_parts(name_ptr, name_len), + b"readUInt8", + "the captured name must spell the method" + ); + } +} + +/// LIVENESS for the two above: they only mean something if the captured +/// pointer is genuinely stable, so pin the property the fix relies on — the +/// `'static` lookup returns the LITERAL out of its own list, never a borrow of +/// the caller's bytes. A future edit that "simplifies" it to `Some(name)` +/// compiles and passes every behavioural test on a lucky allocator; it fails +/// here. +#[test] +fn the_static_method_name_lookup_does_not_borrow_its_argument() { + let owned = String::from("readUInt8"); + let found = crate::object::buffer_method_name_static(&owned) + .expect("readUInt8 is a Buffer method"); + + assert_ne!( + found.as_ptr(), + owned.as_ptr(), + "the lookup returned a borrow of its argument — the whole point is a \ + pointer that outlives the caller's storage" + ); + assert_eq!(found, "readUInt8"); + assert_eq!( + found.as_ptr(), + crate::object::buffer_method_name_static("readUInt8") + .unwrap() + .as_ptr(), + "every caller must get the SAME static literal, whatever storage its \ + own copy of the name lives in" + ); + assert!( + crate::object::buffer_method_name_static("notAMethod").is_none(), + "and a non-method must not resolve" + ); +} diff --git a/crates/perry-runtime/src/gc/tests/mod.rs b/crates/perry-runtime/src/gc/tests/mod.rs index f54eb4fe7d..d155dd3574 100644 --- a/crates/perry-runtime/src/gc/tests/mod.rs +++ b/crates/perry-runtime/src/gc/tests/mod.rs @@ -3,6 +3,7 @@ mod barrier; mod barrier_arming; mod barrier_decoded_parent; mod budgeted_step_api; +mod buffer_bound_method_name; mod buffer_side_tables; mod clone_keys_array_init; mod contract; diff --git a/crates/perry-runtime/src/object/buffer_dispatch.rs b/crates/perry-runtime/src/object/buffer_dispatch.rs index cda5b577ab..1d00e806af 100644 --- a/crates/perry-runtime/src/object/buffer_dispatch.rs +++ b/crates/perry-runtime/src/object/buffer_dispatch.rs @@ -78,152 +78,173 @@ fn validate_buffer_target(value: f64, name: &str) { /// is the raw heap pointer (already stripped of NaN-box tags). Routes /// the Node-style numeric read/write/search/swap method family through /// `crate::buffer` helpers; unknown methods return undefined. -/// Issue #639 followup: list of method names recognized by `dispatch_buffer_method`. -/// Used by `js_object_get_field_by_name`'s Buffer arm to decide whether a -/// non-length property read should synthesize a bound-method closure (so -/// duck-type tests like `typeof v.readUInt8 === "function"` pass and a +/// Issue #639 followup: the method names `dispatch_buffer_method` recognizes, +/// kept as ONE list that generates both the predicate and the `'static` lookup. +/// `js_object_get_field_by_name`'s Buffer arm uses the predicate to decide +/// whether a non-length property read should synthesize a bound-method closure +/// (so duck-type tests like `typeof v.readUInt8 === "function"` pass and a /// subsequent call dispatches through `js_native_call_method`). /// /// Keep this list aligned with the `match method_name` arms below — every /// arm there should be reachable from a method-as-value read. -pub fn is_buffer_method_name(name: &str) -> bool { - matches!( - name, - "toString" - | "inspect" - | "slice" - | "subarray" - | "set" - | "copy" - | "write" - | "toJSON" - | "export" - | "toCryptoKey" - | "fill" - | "equals" - | "compare" - | "indexOf" - | "lastIndexOf" - | "includes" - | "at" - | "swap16" - | "swap32" - | "swap64" - // Issue #1206: explicit iterator-protocol surface. - | "values" - | "keys" - | "entries" - // Object.prototype methods exposed on Buffer instances so - // safer-buffer's `if (buffer.hasOwnProperty(...))` probe (and - // similar duck-type tests in express / body-parser dependents) - // resolve to a callable, not undefined. Without these, - // `typeof buf.hasOwnProperty` is `"undefined"` and the - // subsequent invocation throws "buffer.hasOwnProperty is not - // a function" at express startup. - | "hasOwnProperty" - | "propertyIsEnumerable" - | "valueOf" - | "isPrototypeOf" - | "toLocaleString" - | "readUInt8" - | "readUint8" - | "readInt8" - | "readUInt16BE" - | "readUint16BE" - | "readUInt16LE" - | "readUint16LE" - | "readInt16BE" - | "readInt16LE" - | "readUInt32BE" - | "readUint32BE" - | "readUInt32LE" - | "readUint32LE" - | "readInt32BE" - | "readInt32LE" - | "readFloatBE" - | "readFloatLE" - | "readDoubleBE" - | "readDoubleLE" - | "readBigInt64BE" - | "readBigInt64LE" - | "readBigUInt64BE" - | "readBigUint64BE" - | "readBigUInt64LE" - | "readBigUint64LE" - | "readUIntBE" - | "readUintBE" - | "readUIntLE" - | "readUintLE" - | "readIntBE" - | "readIntLE" - | "writeUInt8" - | "writeUint8" - | "writeInt8" - | "writeUInt16BE" - | "writeUint16BE" - | "writeUInt16LE" - | "writeUint16LE" - | "writeInt16BE" - | "writeInt16LE" - | "writeUInt32BE" - | "writeUint32BE" - | "writeUInt32LE" - | "writeUint32LE" - | "writeInt32BE" - | "writeInt32LE" - | "writeFloatBE" - | "writeFloatLE" - | "writeDoubleBE" - | "writeDoubleLE" - | "writeBigInt64BE" - | "writeBigInt64LE" - | "writeBigUInt64BE" - | "writeBigUint64BE" - | "writeBigUInt64LE" - | "writeBigUint64LE" - | "writeUIntBE" - | "writeUintBE" - | "writeUIntLE" - | "writeUintLE" - | "writeIntBE" - | "writeIntLE" - // #2901: TC39 Uint8Array base64/hex instance conversion APIs. - | "toBase64" - | "toHex" - | "setFromBase64" - | "setFromHex" - // #2879: typed-array mutators that reach buffer dispatch for the - // Uint8Array/Buffer shape. - | "copyWithin" - // #2878: DataView numeric accessors. These resolve as bound-method - // values on a DataView-marked buffer (so `typeof dv.getUint8 === - // "function"`); the call routes through `dispatch_buffer_method`. - | "getInt8" - | "getUint8" - | "getInt16" - | "getUint16" - | "getInt32" - | "getUint32" - | "getFloat32" - | "getFloat64" - | "setInt8" - | "setUint8" - | "setInt16" - | "setUint16" - | "setInt32" - | "setUint32" - | "setFloat32" - | "setFloat64" - // #4365: DataView BigInt64/BigUint64 accessors (8-byte BigInt - // read/write). Route through `dispatch_buffer_method` like the - // other DataView numeric methods. - | "getBigInt64" - | "getBigUint64" - | "setBigInt64" - | "setBigUint64" - ) +macro_rules! buffer_method_names { + ($($name:literal),+ $(,)?) => { + pub fn is_buffer_method_name(name: &str) -> bool { + matches!(name, $($name)|+) + } + + /// The matched name as a `'static` string: the literal out of this + /// list, never a borrow of `name`. + /// + /// `js_class_method_bind` captures the name *pointer* into the bound + /// closure and `dispatch_bound_method` re-reads it at CALL time, so a + /// caller that hands it bytes the caller owns gives the closure a + /// pointer that outlives them. Codegen satisfies that contract with + /// per-module rodata globals; runtime callers satisfy it with this. + pub fn buffer_method_name_static(name: &str) -> Option<&'static str> { + match name { + $($name => Some($name),)+ + _ => None, + } + } + }; } +buffer_method_names!( + "toString", + "inspect", + "slice", + "subarray", + "set", + "copy", + "write", + "toJSON", + "export", + "toCryptoKey", + "fill", + "equals", + "compare", + "indexOf", + "lastIndexOf", + "includes", + "at", + "swap16", + "swap32", + "swap64", + // Issue #1206: explicit iterator-protocol surface. + "values", + "keys", + "entries", + // Object.prototype methods exposed on Buffer instances so + // safer-buffer's `if (buffer.hasOwnProperty(...))` probe (and + // similar duck-type tests in express / body-parser dependents) + // resolve to a callable, not undefined. Without these, + // `typeof buf.hasOwnProperty` is `"undefined"` and the + // subsequent invocation throws "buffer.hasOwnProperty is not + // a function" at express startup. + "hasOwnProperty", + "propertyIsEnumerable", + "valueOf", + "isPrototypeOf", + "toLocaleString", + "readUInt8", + "readUint8", + "readInt8", + "readUInt16BE", + "readUint16BE", + "readUInt16LE", + "readUint16LE", + "readInt16BE", + "readInt16LE", + "readUInt32BE", + "readUint32BE", + "readUInt32LE", + "readUint32LE", + "readInt32BE", + "readInt32LE", + "readFloatBE", + "readFloatLE", + "readDoubleBE", + "readDoubleLE", + "readBigInt64BE", + "readBigInt64LE", + "readBigUInt64BE", + "readBigUint64BE", + "readBigUInt64LE", + "readBigUint64LE", + "readUIntBE", + "readUintBE", + "readUIntLE", + "readUintLE", + "readIntBE", + "readIntLE", + "writeUInt8", + "writeUint8", + "writeInt8", + "writeUInt16BE", + "writeUint16BE", + "writeUInt16LE", + "writeUint16LE", + "writeInt16BE", + "writeInt16LE", + "writeUInt32BE", + "writeUint32BE", + "writeUInt32LE", + "writeUint32LE", + "writeInt32BE", + "writeInt32LE", + "writeFloatBE", + "writeFloatLE", + "writeDoubleBE", + "writeDoubleLE", + "writeBigInt64BE", + "writeBigInt64LE", + "writeBigUInt64BE", + "writeBigUint64BE", + "writeBigUInt64LE", + "writeBigUint64LE", + "writeUIntBE", + "writeUintBE", + "writeUIntLE", + "writeUintLE", + "writeIntBE", + "writeIntLE", + // #2901: TC39 Uint8Array base64/hex instance conversion APIs. + "toBase64", + "toHex", + "setFromBase64", + "setFromHex", + // #2879: typed-array mutators that reach buffer dispatch for the + // Uint8Array/Buffer shape. + "copyWithin", + // #2878: DataView numeric accessors. These resolve as bound-method + // values on a DataView-marked buffer (so `typeof dv.getUint8 === + // "function"`); the call routes through `dispatch_buffer_method`. + "getInt8", + "getUint8", + "getInt16", + "getUint16", + "getInt32", + "getUint32", + "getFloat32", + "getFloat64", + "setInt8", + "setUint8", + "setInt16", + "setUint16", + "setInt32", + "setUint32", + "setFloat32", + "setFloat64", + // #4365: DataView BigInt64/BigUint64 accessors (8-byte BigInt + // read/write). Route through `dispatch_buffer_method` like the + // other DataView numeric methods. + "getBigInt64", + "getBigUint64", + "setBigInt64", + "setBigUint64", +); + unsafe fn buffer_secret_export_format(bits: f64) -> Option { let raw = bits.to_bits(); if (raw >> 48) as u16 == 0x7FFC { diff --git a/crates/perry-runtime/src/object/field_get_set/buffer_own_prop.rs b/crates/perry-runtime/src/object/field_get_set/buffer_own_prop.rs index 927026ef26..b5fb088114 100644 --- a/crates/perry-runtime/src/object/field_get_set/buffer_own_prop.rs +++ b/crates/perry-runtime/src/object/field_get_set/buffer_own_prop.rs @@ -23,18 +23,20 @@ use super::*; pub(super) fn buffer_own_prop_or_method( obj: *const ObjectHeader, key_bytes: &[u8], - key_ptr: *const u8, - key_len: usize, ) -> Option { let name = std::str::from_utf8(key_bytes).ok()?; if let Some(v) = crate::buffer::buffer_get_own_prop(obj as usize, name) { return Some(JSValue::from_bits(v.to_bits())); } - if crate::object::buffer_dispatch::is_buffer_method_name(name) { + // The bound closure keeps the name POINTER and re-reads it at call time + // (`dispatch_bound_method`), so it must not point into the key string: + // that is a movable GC heap allocation, and `key_bytes` borrows its + // interior. Bind the `'static` literal instead. + if let Some(method) = crate::object::buffer_dispatch::buffer_method_name_static(name) { let bound = crate::object::js_class_method_bind( crate::value::js_nanbox_pointer(obj as i64), - key_ptr, - key_len, + method.as_ptr(), + method.len(), ); return Some(JSValue::from_bits(bound.to_bits())); } 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 4724ba8751..ef16068da2 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 @@ -257,9 +257,9 @@ pub(crate) fn get_field_by_name_object_tail( } // An own property on the Buffer shadows the same-named prototype // method; both reads live in `buffer_own_prop`. - if let Some(v) = super::buffer_own_prop::buffer_own_prop_or_method( - obj, key_bytes, key_ptr, key_len, - ) { + if let Some(v) = + super::buffer_own_prop::buffer_own_prop_or_method(obj, key_bytes) + { return v; } // ArrayBuffer.prototype `resizable` / `maxByteLength` getters. diff --git a/crates/perry-runtime/src/object/polymorphic_index.rs b/crates/perry-runtime/src/object/polymorphic_index.rs index c2bd178054..08f0f54246 100644 --- a/crates/perry-runtime/src/object/polymorphic_index.rs +++ b/crates/perry-runtime/src/object/polymorphic_index.rs @@ -185,12 +185,16 @@ pub extern "C" fn js_object_get_index_polymorphic(obj_handle: i64, idx: f64) -> if let Some(v) = crate::buffer::buffer_get_own_prop(raw as usize, &name) { return v; } - if crate::object::buffer_dispatch::is_buffer_method_name(&name) { - let bytes = name.as_bytes(); + // The bound closure keeps the name POINTER and re-reads it at + // call time, so it must not borrow from `name` — a local + // `String` freed the moment this returns. + if let Some(method) = + crate::object::buffer_dispatch::buffer_method_name_static(&name) + { return crate::object::js_class_method_bind( crate::value::js_nanbox_pointer(raw as i64), - bytes.as_ptr(), - bytes.len(), + method.as_ptr(), + method.len(), ); } }