-
-
Notifications
You must be signed in to change notification settings - Fork 161
fix(runtime): fix Buffer.prototype's own-property shape #10644
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
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,3 @@ | ||
| ### Fixed | ||
|
|
||
| - `Buffer.prototype`'s own property set no longer contains 36 bogus entries (including two bare string literals, `"function"` and `"undefined"`, plus `DataView`/`Uint8Array.prototype`/`Object.prototype` methods that belong further up the prototype chain) and now includes Node's internal `<encoding>Slice`/`<encoding>Write` methods and the deprecated `offset`/`parent` accessors, matching `Object.getOwnPropertyNames(Buffer.prototype)` on Node 26.5.1 exactly (96 own properties, 95 enumerable via `for-in`). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -426,6 +426,83 @@ extern "C" fn buffer_prototype_method_thunk(_closure: *const crate::closure::Clo | |
| f64::from_bits(crate::value::TAG_UNDEFINED) | ||
| } | ||
|
|
||
| /// `this` for a `Buffer.prototype.offset`/`.parent` accessor read, resolved | ||
| /// through `IMPLICIT_THIS` (the same mechanism `require_webcrypto_this` in | ||
| /// `ctor_thunks.rs` uses for Web Crypto getters). `None` for a non-buffer | ||
| /// receiver — Node's real getters answer `undefined` rather than throwing | ||
| /// (`isInstance(this, Buffer) ? … : undefined`), and ordinary Buffer/typed- | ||
| /// array instance reads never reach this getter at all (they resolve | ||
| /// `.offset`/`.parent` directly — see `get_field_by_name_tail.rs`); this only | ||
| /// matters for reflection (`Object.getOwnPropertyDescriptor(Buffer.prototype, | ||
| /// "offset").get.call(x)`) and enumeration. | ||
| fn buffer_prototype_this_addr() -> Option<usize> { | ||
| let this_bits = crate::object::IMPLICIT_THIS.with(|c| c.get()); | ||
| let jv = crate::value::JSValue::from_bits(this_bits); | ||
| if !jv.is_pointer() { | ||
| return None; | ||
| } | ||
| let addr = (this_bits & crate::value::POINTER_MASK) as usize; | ||
| if addr == 0 || crate::buffer::js_buffer_is_buffer(addr as i64) == 0 { | ||
| return None; | ||
| } | ||
| Some(addr) | ||
| } | ||
|
|
||
| /// #10426: `Buffer.prototype.parent` — deprecated legacy alias for | ||
| /// `.buffer` (the backing `ArrayBuffer`), still a real own accessor on | ||
| /// Node's `Buffer.prototype`. | ||
| extern "C" fn buffer_prototype_parent_getter_thunk( | ||
| _closure: *const crate::closure::ClosureHeader, | ||
| ) -> f64 { | ||
| match buffer_prototype_this_addr() { | ||
| Some(addr) => { | ||
| crate::value::js_nanbox_pointer(crate::buffer::buffer_backing_array_buffer(addr) as i64) | ||
| } | ||
| None => f64::from_bits(crate::value::TAG_UNDEFINED), | ||
| } | ||
| } | ||
|
|
||
| /// #10426: `Buffer.prototype.offset` — deprecated legacy alias for | ||
| /// `.byteOffset`, still a real own accessor on Node's `Buffer.prototype`. | ||
| extern "C" fn buffer_prototype_offset_getter_thunk( | ||
| _closure: *const crate::closure::ClosureHeader, | ||
| ) -> f64 { | ||
| match buffer_prototype_this_addr() { | ||
| Some(addr) => crate::buffer::buffer_byte_offset(addr) as f64, | ||
| None => f64::from_bits(crate::value::TAG_UNDEFINED), | ||
| } | ||
| } | ||
|
|
||
| /// Install a `Buffer.prototype` accessor (`offset`/`parent`) — `{ | ||
| /// enumerable: true, configurable: false }`, matching Node's | ||
| /// `ObjectDefineProperty(Buffer.prototype, name, { enumerable: true, get() | ||
| /// {…} })` (no `configurable: true`, so it defaults false). Mirrors | ||
| /// `install_webcrypto_proto_getter`'s shape for a `*mut ObjectHeader` proto. | ||
| fn install_buffer_prototype_getter(proto_obj: *mut ObjectHeader, name: &str, func_ptr: *const u8) { | ||
| if proto_obj.is_null() { | ||
| return; | ||
| } | ||
| crate::closure::js_register_closure_arity(func_ptr, 0); | ||
| let closure = crate::closure::js_closure_alloc(func_ptr, 0); | ||
| let value = if closure.is_null() { | ||
| f64::from_bits(crate::value::TAG_UNDEFINED) | ||
| } else { | ||
| set_bound_native_closure_name(closure, &format!("get {name}")); | ||
| crate::value::js_nanbox_pointer(closure as i64) | ||
| }; | ||
| let key = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); | ||
| js_object_set_field_by_name(proto_obj, key, f64::from_bits(crate::value::TAG_UNDEFINED)); | ||
| super::set_builtin_accessor_descriptor( | ||
| proto_obj as usize, | ||
| name.to_string(), | ||
| super::AccessorDescriptor { | ||
| get: value.to_bits(), | ||
| set: 0, | ||
| }, | ||
| super::PropertyAttrs::new(true, true, false), | ||
| ); | ||
|
Comment on lines
+481
to
+503
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 '400,525p' crates/perry-runtime/src/object/native_module/callable_exports.rs
rg -n 'RuntimeHandleScope|set_bound_native_closure_name|set_builtin_accessor_descriptor|install_.*getter' crates/perry-runtime/src/object crates/perry-runtime/src/closure crates/perry-runtime/src/stringRepository: PerryTS/perry Length of output: 50369 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- RuntimeHandleScope API ---'
rg -n -A45 -B10 'pub (struct|impl).*RuntimeHandleScope|fn root_.*(ptr|string)|refreshed_nanbox|root_nanbox|root_object|root_string' crates/perry-runtime/src/gc crates/perry-runtime/src/object crates/perry-runtime/src/string | head -n 260
printf '%s\n' '--- native closure naming and install patterns ---'
sed -n '1400,1610p' crates/perry-runtime/src/object/native_module.rs
sed -n '990,1045p' crates/perry-runtime/src/object/global_this/proto_methods.rs
sed -n '185,220p' crates/perry-runtime/src/object/regex_proto_thunks.rs
sed -n '235,270p' crates/perry-runtime/src/object/collection_proto_thunks.rs
printf '%s\n' '--- generic getter installation ---'
sed -n '500,555p' crates/perry-runtime/src/object/object_ops/keys_array.rs
sed -n '1630,1710p' crates/perry-runtime/src/object/descriptor_state.rs
printf '%s\n' '--- direct field setter and allocation contracts ---'
rg -n -A35 -B10 'fn js_object_set_field_by_name|pub.*js_object_set_field_by_name|fn js_closure_alloc|pub.*js_closure_alloc|fn js_string_from_bytes|pub.*js_string_from_bytes' crates/perry-runtime/src
printf '%s\n' '--- buffer getter activation ---'
rg -n -A25 -B25 'install_buffer_prototype_getter|BUFFER_STATIC_METHODS|offset.*parent|parent.*offset' crates/perry-runtime/src/object/native_module/callable_exports.rs crates/perry-runtime/src/object/global_thisRepository: PerryTS/perry Length of output: 50370 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- RuntimeHandleScope definition and root methods ---'
rg -n 'pub struct RuntimeHandleScope|impl RuntimeHandleScope|root_raw_mut_ptr|root_raw_ptr|root_string_ptr|root_nanbox_f64|struct RuntimeHandle' crates/perry-runtime/src/gc crates/perry-runtime/src | head -n 180
printf '%s\n' '--- relevant implementation slices ---'
rg -l 'pub struct RuntimeHandleScope|impl RuntimeHandleScope' crates/perry-runtime/src/gc crates/perry-runtime/src | head -n 3 | while read f; do echo "FILE:$f"; grep -n -A180 -B10 'pub struct RuntimeHandleScope\|impl RuntimeHandleScope' "$f" | head -n 420; done
printf '%s\n' '--- exact callee signatures and bodies ---'
rg -n -A80 -B15 'pub unsafe fn js_object_set_field_by_name|pub.*fn js_object_set_field_by_name|fn js_closure_alloc|pub.*fn js_closure_alloc|fn js_string_from_bytes|pub.*fn js_string_from_bytes|fn set_bound_native_closure_name' crates/perry-runtime/src/object crates/perry-runtime/src/closure crates/perry-runtime/src/string crates/perry-runtime/src/value | head -n 420
printf '%s\n' '--- accessor activation context ---'
rg -n -A35 -B20 'install_buffer_prototype_getter\(' crates/perry-runtime/src/object/native_module/callable_exports.rsRepository: PerryTS/perry Length of output: 50369 🏁 Script executed: #!/bin/bash
set -e
printf '%s\n' '--- RuntimeHandle accessors ---'
sed -n '264,390p' crates/perry-runtime/src/gc/roots/runtime_handles.rs
printf '%s\n' '--- closure naming ---'
rg -n -A95 -B12 'pub.*fn set_bound_native_closure_name|fn set_bound_native_closure_name' crates/perry-runtime/src/object/native_module.rs
printf '%s\n' '--- string allocator ---'
rg -n 'pub.*fn js_string_from_bytes|fn js_string_from_bytes' crates/perry-runtime/src/string
sed -n '940,1010p' crates/perry-runtime/src/string/mod.rs
printf '%s\n' '--- object setter ---'
rg -n 'pub.*fn js_object_set_field_by_name|fn js_object_set_field_by_name' crates/perry-runtime/src/object
printf '%s\n' '--- buffer call sites ---'
rg -n -A22 -B22 'install_buffer_prototype_getter' crates/perry-runtime/src/object/native_module/callable_exports.rsRepository: PerryTS/perry Length of output: 5960 🏁 Script executed: #!/bin/bash
printf '%s\n' '--- RuntimeHandle post-collection accessors ---'
sed -n '390,520p' crates/perry-runtime/src/gc/roots/runtime_handles.rs
printf '%s\n' '--- closure naming binding ---'
rg -n -A70 -B15 'set_bound_native_closure_name' crates/perry-runtime/src/object/native_module.rs crates/perry-runtime/src/object/native_module* crates/perry-runtime/src | head -n 180
printf '%s\n' '--- field setter binding ---'
rg -n -A100 -B20 'js_object_set_field_by_name' crates/perry-runtime/src/object crates/perry-runtime/src/field* | head -n 240
printf '%s\n' '--- string allocator binding ---'
rg -n -A45 -B15 'js_string_from_bytes' crates/perry-runtime/src/string | head -n 180Repository: PerryTS/perry Length of output: 50369 🏁 Script executed: #!/bin/bash
printf '%s\n' '--- js_object_set_field_by_name declaration and callers ---'
rg -n 'js_object_set_field_by_name' crates/perry-runtime/src/object --glob '*.rs' | head -n 80
printf '%s\n' '--- likely implementation files ---'
git ls-files 'crates/perry-runtime/src/object' | grep -E 'field|set|mod.rs' | head -n 80Repository: PerryTS/perry Length of output: 11757 🏁 Script executed: #!/bin/bash
sed -n '1,180p' crates/perry-runtime/src/object/field_set_by_name.rs
sed -n '1,180p' crates/perry-runtime/src/object/field_set_by_name/tail.rs
sed -n '1,160p' crates/perry-runtime/src/object/field_set_by_name/write_helpers.rsRepository: PerryTS/perry Length of output: 25306 Refresh prototype and getter handles after every allocation.
Create a 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| const BUFFER_STATIC_METHODS: &[&str] = &[ | ||
| "from", | ||
| "alloc", | ||
|
|
@@ -440,22 +517,40 @@ const BUFFER_STATIC_METHODS: &[&str] = &[ | |
| "copyBytesFrom", | ||
| ]; | ||
|
|
||
| /// Node exposes the WHOLE Buffer method surface on `Buffer.prototype`, and it is | ||
| /// enumerable — `for (const k in Buffer.prototype)` yields ~93 names there. | ||
| /// Perry used to install ELEVEN, which quietly broke any code that walks the | ||
| /// prototype: mysql2 sizes every outgoing packet by no-op'ing the write methods | ||
| /// of a zero-length Buffer | ||
| /// Node's actual `Buffer.prototype` own-property surface — checked | ||
| /// property-by-property against `Object.getOwnPropertyNames(Buffer.prototype)` | ||
| /// on Node 26.5.1 (96 names there; 93 real callable methods here, plus | ||
| /// `constructor` and the `offset`/`parent` accessors installed separately | ||
| /// below = 96). Perry used to install ELEVEN, which quietly broke any code | ||
| /// that walks the prototype: mysql2 sizes every outgoing packet by no-op'ing | ||
| /// the write methods of a zero-length Buffer | ||
| /// (`for (const k in Buffer.prototype) if (typeof mock[k] === "function") mock[k] = noop`), | ||
| /// so `writeUInt32LE` — absent from the stub list — stayed live, wrote into the | ||
| /// empty measuring buffer, and killed the MySQL handshake with | ||
| /// RangeError [ERR_OUT_OF_RANGE]. Generated from the dispatcher's own | ||
| /// `is_buffer_method_name` table so the two can't drift. | ||
| /// RangeError [ERR_OUT_OF_RANGE]. | ||
| /// | ||
| /// #10426: this list is DELIBERATELY NOT the same as | ||
| /// `buffer_dispatch::is_buffer_method_name` (a comment here used to say it | ||
| /// was "generated from" that table "so the two can't drift" — that coupling | ||
| /// was the bug). `is_buffer_method_name` answers a different question — "does | ||
| /// a property read on a Buffer *instance* need to synthesize a bound-method | ||
| /// closure" — and is deliberately broad: it also recognizes names Buffer | ||
| /// instances answer only by INHERITANCE (`Uint8Array.prototype.at`/`set`/ | ||
| /// `entries`/`keys`/`values`/`copyWithin`/`toBase64`/`toHex`/`setFromBase64`/ | ||
| /// `setFromHex`), by DataView accessors on a DataView-marked buffer | ||
| /// (`getInt32`/`setFloat64`/…), and by `Object.prototype` | ||
| /// (`hasOwnProperty`/`isPrototypeOf`/`propertyIsEnumerable`/`valueOf`) so | ||
| /// duck-type probes on an INSTANCE keep working (see that table's own | ||
| /// comments). None of those belong on `Buffer.prototype` itself as OWN | ||
| /// properties — Node inherits them further up the chain — so installing this | ||
| /// list from that one copied 36 names Node never puts here (plus two bare | ||
| /// string literals, `"function"` and `"undefined"`, that had drifted in from | ||
| /// nearby prose/JS-idiom comments and were never real method names at all). | ||
| const BUFFER_PROTOTYPE_METHODS: &[&str] = &[ | ||
| "toString", | ||
| "inspect", | ||
| "slice", | ||
| "subarray", | ||
| "set", | ||
| "copy", | ||
| "write", | ||
| "toJSON", | ||
|
|
@@ -465,18 +560,9 @@ const BUFFER_PROTOTYPE_METHODS: &[&str] = &[ | |
| "indexOf", | ||
| "lastIndexOf", | ||
| "includes", | ||
| "at", | ||
| "swap16", | ||
| "swap32", | ||
| "swap64", | ||
| "values", | ||
| "keys", | ||
| "entries", | ||
| "undefined", | ||
| "hasOwnProperty", | ||
| "propertyIsEnumerable", | ||
| "valueOf", | ||
| "isPrototypeOf", | ||
| "toLocaleString", | ||
| "readUInt8", | ||
| "readUint8", | ||
|
|
@@ -540,32 +626,20 @@ const BUFFER_PROTOTYPE_METHODS: &[&str] = &[ | |
| "writeUintLE", | ||
| "writeIntBE", | ||
| "writeIntLE", | ||
| "toBase64", | ||
| "toHex", | ||
| "setFromBase64", | ||
| "setFromHex", | ||
| "copyWithin", | ||
| "function", | ||
| "getInt8", | ||
| "getUint8", | ||
| "getInt16", | ||
| "getUint16", | ||
| "getInt32", | ||
| "getUint32", | ||
| "getFloat32", | ||
| "getFloat64", | ||
| "setInt8", | ||
| "setUint8", | ||
| "setInt16", | ||
| "setUint16", | ||
| "setInt32", | ||
| "setUint32", | ||
| "setFloat32", | ||
| "setFloat64", | ||
| "getBigInt64", | ||
| "getBigUint64", | ||
| "setBigInt64", | ||
| "setBigUint64", | ||
| "asciiSlice", | ||
| "asciiWrite", | ||
| "base64Slice", | ||
| "base64Write", | ||
| "base64urlSlice", | ||
| "base64urlWrite", | ||
| "hexSlice", | ||
| "hexWrite", | ||
| "latin1Slice", | ||
| "latin1Write", | ||
| "ucs2Slice", | ||
| "ucs2Write", | ||
| "utf8Slice", | ||
| "utf8Write", | ||
| ]; | ||
|
|
||
| const SQLITE_DATABASE_SYNC_PROTOTYPE_METHODS: &[&str] = &[ | ||
|
|
@@ -950,6 +1024,18 @@ pub(crate) fn buffer_constructor_value() -> f64 { | |
| }) | ||
| }); | ||
| } | ||
| proto.with_mut_ptr(|proto: *mut ObjectHeader| { | ||
| install_buffer_prototype_getter( | ||
| proto, | ||
| "parent", | ||
| buffer_prototype_parent_getter_thunk as *const u8, | ||
| ); | ||
| install_buffer_prototype_getter( | ||
| proto, | ||
| "offset", | ||
| buffer_prototype_offset_getter_thunk as *const u8, | ||
| ); | ||
| }); | ||
| let proto_value = proto.with_mut_ptr(|proto: *mut ObjectHeader| { | ||
| crate::value::js_nanbox_pointer(proto as i64) | ||
| }); | ||
|
|
||
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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 19167
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 50370
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 16991
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 9906
🌐 Web query:
official Node.js source Buffer.prototype utf8Slice utf8Write ParseArrayIndex current💡 Result:
<search_synthesis>
The current official Node.js implementation is in
src/node_buffer.ccon themainbranch: https://github.com/nodejs/node/blob/main/src/node_buffer.cc. It containsParseArrayIndex, which treatsundefinedas the supplied default, converts other values with V8IntegerValue(), rejects negative values, rejects values that do not fit insize_t, and writes the result through the output pointer. The helper returns an emptyMaybewhen conversion throws andJust(false)for out-of-bounds input. [1] The same native source implements the UTF-8 slice path (StringSlice, exposed through the internal buffer binding). It validates the receiver as a Buffer/ArrayBufferView, defaultsstartto 0 andendto the buffer length, callsParseArrayIndexfor both, returns an empty string whenend <= start, and decodes the selected byte range with Node’s UTF-8 string conversion. [1]utf8Sliceand the corresponding encoding-specific write methods are also represented in Node’s internal TypeScript declarations:utf8Slice(start, end): string, while the declarations shown for encoding-specific writes include methods such asbase64Write,hexWrite, anducs2Write; UTF-8 writing is exposed through the native/static binding asutf8WriteStatic. [2] The historical JavaScript encoding dispatch explicitly routes UTF-8 writes tobuf.utf8Write(string, offset, len)and UTF-8 slices tobuf.utf8Slice(start, end). [3] For public API guidance, current Node documentation recommends the documentedbuf.toString([encoding[, start[, end]]])andbuf.write(string[, offset[, length]][, encoding])APIs rather than relying on these long-standing encoding-specific methods. Node’s documentation states that UTF-8 is the default encoding and that invalid UTF-8 bytes decode as U+FFFD. [4] A current Node pull request specifically proposes documenting the previously public encoding-specificBuffer.prototype.*Slice()and*Write()methods, while recommendingbuf.toString()/buf.write()for common use. [5] In short: for current source inspection, usesrc/node_buffer.ccforParseArrayIndexand the native UTF-8 slice/write implementations; uselib/buffer.js/the historical encoding-dispatch code to see howBuffer.prototypemethods route toutf8Sliceandutf8Write; and usebuf.toString('utf8', start, end)plusbuf.write(str, offset, length, 'utf8')as the stable public interfaces. [3][1][5][6]</search_synthesis>
<source_evidence>
Citations:
🏁 Script executed:
Repository: PerryTS/perry
Length of output: 9534
Match Node’s
ParseArrayIndexsemantics for fixed slice/write arguments.arg_i32does not distinguish explicitundefinedfrom zero. Therefore,utf8Slice(1, undefined)passesend = 0instead of the buffer length, andutf8Write("x", 0, undefined)passesmax_len = 0instead of the remaining capacity.The fixed slice path also clamps negative starts and oversized ends in
js_buffer_to_string_range. The fixed write path clamps negative or oversized offsets injs_buffer_write_len. Node rejects negative indices and invalid slice ends or write offsets with itsParseArrayIndexchecks. Node still limits a valid write length to the remaining capacity; do not change that behavior to reject oversized lengths.Add equivalent
undefineddefaults and validation before calling these helpers.🤖 Prompt for AI Agents