From e699da9b9f4d73918e023277bd6cb11dded7fe75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Fri, 19 Jun 2026 21:34:59 +0200 Subject: [PATCH 1/3] fix(hir+runtime): native-instance values support arbitrary own properties (read default = property GET, not invoking method call) The native-instance bare-member-read block in expr_member.rs ended in an unconditional invoking NativeMethodCall fallback, so a bare property READ on a value the HIR tagged as a native instance became a 0-arg method CALL. Any module not covered by the ~20 per-module special-case arms invoked the property: - reading an array prop -> 'value is not a function' - reading a function prop -> invoked with 0 args -> downstream undefined deref Invert the default: a bare native-instance member read lowers to PropertyGet unless the property is a known native method/getter that must dispatch (consolidated into is_native_dispatch_member: blob/fetch data getters, classic + web-stream state getters, http/net/events/dns/dgram/inspector/sqlite/url/worker/ util/console/Headers members). Real method CALLS go through the call-expression path and are unaffected. Pair the read fix with a write side-table (object/handle_expando.rs): a GC-scanned per-handle expando map so own-property writes to native handles (Blob/Response) persist and read back, mirroring closures' CLOSURE_PROPS. Fixes arbitrary own-property read+write on native instances (e.g. the bundled debug package's createDebug.colors / .init). Adds e2e regression coverage. --- crates/perry-hir/src/lower/expr_member.rs | 291 +++++++++++++++++- crates/perry-runtime/src/gc/mod.rs | 6 + .../src/object/handle_expando.rs | 151 +++++++++ crates/perry-runtime/src/object/mod.rs | 4 +- crates/perry-stdlib/src/common/dispatch.rs | 27 ++ .../tests/native_instance_own_property.rs | 213 +++++++++++++ 6 files changed, 686 insertions(+), 6 deletions(-) create mode 100644 crates/perry-runtime/src/object/handle_expando.rs create mode 100644 crates/perry/tests/native_instance_own_property.rs diff --git a/crates/perry-hir/src/lower/expr_member.rs b/crates/perry-hir/src/lower/expr_member.rs index 87824480c4..1f2077c23e 100644 --- a/crates/perry-hir/src/lower/expr_member.rs +++ b/crates/perry-hir/src/lower/expr_member.rs @@ -1090,8 +1090,10 @@ fn lower_member_inner(ctx: &mut LoweringContext, member: &ast::MemberExpr) -> Re } else { None } + } else if let Some(func_id) = ctx.lookup_func(&fn_name) { + Some(Expr::FuncRef(func_id)) } else { - ctx.lookup_func(&fn_name).map(Expr::FuncRef) + None }; if let Some(func_expr) = func_expr { return Ok(Expr::GetFunctionPrototypeMethod { @@ -1191,8 +1193,54 @@ fn lower_member_inner(ctx: &mut LoweringContext, member: &ast::MemberExpr) -> Re | "writable_stream_writer" ) && !is_stream_api_member(&module_name, &property_name) { - // Fall through — let the regular member access path - // below handle the user-declared subclass field. + // Issue #562 + #wall (debug `_.colors`): a Web-Streams + // instance / subclass carries the bare-stream tag for + // inherited-method dispatch but ALSO holds user-declared + // own fields (`w.seenLengths`, `x.colors = [...]`). A bare + // read of any name that is NOT a known Web-Streams API + // method/getter must be a plain own-property GET — NOT the + // 0-arg NativeMethodCall fallback (which would *invoke* the + // stored value). The matching write lowers to a generic + // PropertySet that the runtime routes to the per-handle + // expando side-table (`object/handle_expando.rs`), so the + // value persists and reads back. Real stream methods/getters + // (handled by `is_stream_api_member` above) keep dispatching. + let object_expr = lower_expr(ctx, &member.obj)?; + return Ok(Expr::PropertyGet { + object: Box::new(object_expr), + property: property_name, + }); + } else if module_name == "blob" && !is_blob_getter_name(&property_name) { + // #wall (debug `_.colors`): a Blob/File instance is a real + // heap object that also carries user-assigned OWN properties + // (library bookkeeping fields, `x.colors = [...]`, etc.). A + // bare read of any name that is NOT a known native data + // getter (`size`/`type`/`name`/`lastModified`, handled by the + // generic fallback's 0-arg NativeMethodCall → FFI dispatch) + // must be a plain own-property GET — NOT the invoking + // fallback, which lowers to `js_native_call_method_nullsafe( + // inst, "", 0 args)` and *calls* the stored value (an + // array → `TypeError: value is not a function`). Real Blob + // *methods* (`x.text()`, `x.slice()`, `x.arrayBuffer()`) + // arrive through the call-expression path, not this bare + // read, so they still dispatch. + let object_expr = lower_expr(ctx, &member.obj)?; + return Ok(Expr::PropertyGet { + object: Box::new(object_expr), + property: property_name, + }); + } else if module_name == "fetch" && !is_fetch_response_getter_name(&property_name) { + // Same heap-object rule for fetch `Response` instances: an + // arbitrary property read that is not a known Response data + // getter (`status`/`ok`/`headers`/…) must be a plain + // own-property GET, not an invoking 0-arg native call. Body + // methods (`res.json()`/`res.text()`/`res.clone()`) come in + // via the call path and keep dispatching. + let object_expr = lower_expr(ctx, &member.obj)?; + return Ok(Expr::PropertyGet { + object: Box::new(object_expr), + property: property_name, + }); } else if matches!(module_name.as_str(), "util" | "sys") && matches!(class_name.as_str(), "MIMEType" | "MIMEParams") { @@ -1241,6 +1289,30 @@ fn lower_member_inner(ctx: &mut LoweringContext, member: &ast::MemberExpr) -> Re object: Box::new(object_expr), property: property_name, }); + } else if matches!(module_name.as_str(), "stream" | "node:stream") + && !is_classic_stream_getter_name(&property_name) + { + // #wall (debug `_.colors`): a classic Node stream instance + // is a heap object that also carries user-assigned OWN + // properties (`_.colors = [...]`, library bookkeeping fields, + // etc.). A bare read of any name that is NOT a known stream + // method (handled by the arm above) and NOT a known stream + // property getter (the allowlist below) must be a plain + // own-property GET — NOT the 0-arg NativeMethodCall fallback, + // which lowers to `js_native_call_method_nullsafe(inst, + // "", 0 args)` and *invokes* the stored value + // (`_.colors` is an array → `TypeError: value is not a + // function`). Node just returns the property. The matching + // write (`_.colors = v`) already lowers to a generic + // PropertySet on the heap object, so the value persists and + // reads back. Known getters (`destroyed`, `readableLength`, + // …) still keep the NativeMethodCall fallback below so the + // codegen NativeModSig table dispatches them to their FFI. + let object_expr = lower_expr(ctx, &member.obj)?; + return Ok(Expr::PropertyGet { + object: Box::new(object_expr), + property: property_name, + }); } else if module_name == "net" && matches!(class_name.as_str(), "Socket" | "Stream") && is_net_socket_method_name(&property_name) @@ -1609,7 +1681,31 @@ fn lower_member_inner(ctx: &mut LoweringContext, member: &ast::MemberExpr) -> Re object: Box::new(object_expr), property: property_name, }); - } else { + } else if is_native_dispatch_member( + &module_name, + &class_name, + &property_name, + ) { + // #wall (debug `_.colors` / `_.init`): INVERTED DEFAULT. + // A bare native-instance member READ now defaults to a + // plain `PropertyGet` (the final `else` below). It only + // reaches this INVOKING `NativeMethodCall { args: [] }` + // dispatch path when the property is a *known* native + // method/getter that must dispatch through the codegen + // NATIVE_MODULE_TABLE / per-class FFI + // (`is_native_dispatch_member`). Previously this arm was + // the unconditional catch-all `else`, so any value the + // HIR mis-tagged native under a module NOT covered by the + // per-module arms above (the bundled `debug` package's + // `createDebug` value) had EVERY non-method property read + // lowered to a 0-arg native call that *invoked* the + // stored value — `_.colors` (an array) → + // `value is not a function`; `_.init` (a function) called + // with 0 args → `Cannot set properties of null`. Real + // native method calls `x.method(args)` arrive via the + // call-expression path (local_natives.rs / lower_call), + // NOT this bare-read block, so they are unaffected. + // // Issue #577 — `req.method` / `res.statusCode` etc. // get rewritten to `__get_` so the property // read dispatches through NATIVE_MODULE_TABLE entries @@ -1707,6 +1803,24 @@ fn lower_member_inner(ctx: &mut LoweringContext, member: &ast::MemberExpr) -> Re method: property_name, args: Vec::new(), }); + } else { + // INVERTED DEFAULT (#wall debug `_.colors` / `_.init`): a + // bare native-instance member read that is NOT a known + // native dispatch member is a plain own-property GET — it + // READS the stored value instead of INVOKING it as a 0-arg + // native method call. This is what makes a value the HIR + // mis-tagged native under an uncovered module (the bundled + // `debug` package's `createDebug`) read `_.colors` / `_.init` + // as ordinary properties (Node's behaviour) rather than + // calling them. Writes to native handles already lower to a + // generic PropertySet routed through the per-handle expando + // side-table (`object/handle_expando.rs`), so the value + // persists and reads back here. + let object_expr = lower_expr(ctx, &member.obj)?; + return Ok(Expr::PropertyGet { + object: Box::new(object_expr), + property: property_name, + }); } } } @@ -2801,6 +2915,140 @@ pub(super) fn stdlib_namespace_receiver( /// property access. Mirrors the methods + accessors hardcoded in /// `crates/perry-codegen/src/lower_call.rs`'s /// `module == ""` arms. +/// Native data-property getters exposed by `blob`-module instances (Blob / +/// File). A bare read of one of these must keep the 0-arg NativeMethodCall +/// dispatch so codegen routes it to the FFI getter (`js_blob_size`, …). +/// Everything else read off a Blob instance is a user-assigned own property +/// and must lower to a plain PropertyGet (see the heap-object guard in +/// `lower_member`). +/// #wall (debug `_.colors` / `_.init`): the inverted-default predicate for the +/// native-instance bare-member-READ block in `lower_member`. Returns `true` only +/// when `(module, class, property)` is a *known* native method/getter that must +/// dispatch through the codegen NATIVE_MODULE_TABLE / per-class FFI as a 0-arg +/// `NativeMethodCall`. Everything else (own properties, library bookkeeping +/// fields, and — critically — any value the HIR mis-tagged native under a module +/// NOT covered by the per-module arms, like the bundled `debug` package's +/// `createDebug`) falls through to a plain `PropertyGet` that READS the stored +/// value instead of INVOKING it. +/// +/// This is the consolidated set of the genuine native members that legitimately +/// reach the dispatching arm: the data getters whose values come from FFI +/// (`blob.size`, `res.status`, classic/web-stream state getters), the HTTP +/// per-class FFI getters / methods that are rewritten to `__get_` or +/// dispatched by class_filter, and the events/net method sets. Method-VALUE +/// reads that the per-module arms above already lower to `PropertyGet` are NOT +/// listed here — they keep reading as bound-method values, and the call form +/// `x.method(args)` goes through the call-expression path, unaffected. +fn is_native_dispatch_member(module: &str, class: &str, prop: &str) -> bool { + match module { + // Data getters resolved by FFI. + "blob" => is_blob_getter_name(prop), + "fetch" => is_fetch_response_getter_name(prop), + // Web Streams: only the getter list reaches dispatch (methods are + // PropertyGet bound-method reads). + "readable_stream" + | "writable_stream" + | "transform_stream" + | "readable_stream_reader" + | "writable_stream_writer" => { + is_stream_api_member(module, prop) + && matches!( + prop, + "locked" + | "desiredSize" + | "closed" + | "ready" + | "readable" + | "writable" + | "byobRequest" + ) + } + // Classic Node streams: state getters dispatch; methods read as values. + "stream" | "node:stream" => is_classic_stream_getter_name(prop), + // HTTP / HTTPS: the per-class FFI getters (rewritten to `__get_`) + // and the runtime/method property sets that dispatch through the + // NATIVE_MODULE_TABLE class_filter path. + "http" | "https" => match class { + "IncomingMessage" => { + is_http_incoming_message_runtime_property_name(prop) + || is_http_incoming_message_method_name(prop) + || matches!(prop, "statusCode" | "statusMessage" | "headers") + } + "ServerResponse" => { + is_http_server_response_runtime_property_name(prop) + || is_http_server_response_method_name(prop) + } + "ClientRequest" => { + is_http_client_request_method_name(prop) + || matches!( + prop, + "method" + | "protocol" + | "host" + | "path" + | "aborted" + | "connection" + | "destroyed" + | "finished" + | "maxHeadersCount" + | "reusedSocket" + | "socket" + | "writableEnded" + | "writableFinished" + ) + } + "HttpServer" | "HttpsServer" => matches!( + prop, + "listening" + | "headersTimeout" + | "keepAliveTimeout" + | "keepAliveTimeoutBuffer" + | "requestTimeout" + | "timeout" + | "maxHeadersCount" + | "maxRequestsPerSocket" + ), + "Agent" => matches!(prop, "createConnection" | "createSocket"), + _ => true, + }, + // events / net instances dispatch their EventEmitter / socket methods + // and getters through the class_filter table. These modules expose no + // user own-property surface in the bundle walls, so keep dispatching + // for any member to preserve existing behaviour. + "events" | "net" => true, + // Other native modules historically routed every uncovered member to + // the dispatching fallback. They have no observed user-own-property + // surface, so preserve that: dispatch any member not handled by the + // PropertyGet arms above. + "dns" | "dns/promises" | "dgram" | "inspector" | "inspector/promises" | "sqlite" + | "url" | "worker_threads" | "util" | "sys" | "console" | "Headers" => true, + // Any other module (e.g. a mis-tagged `debug` createDebug value): a bare + // member read is an own-property GET, never an invoking dispatch. + _ => false, + } +} + +fn is_blob_getter_name(prop: &str) -> bool { + matches!(prop, "size" | "type" | "name" | "lastModified") +} + +/// Native data-property getters exposed by `fetch`-module Response instances. +/// Mirrors the property arms in `perry-codegen` `lower_call/options/fetch.rs`. +fn is_fetch_response_getter_name(prop: &str) -> bool { + matches!( + prop, + "status" + | "statusText" + | "ok" + | "type" + | "url" + | "redirected" + | "bodyUsed" + | "headers" + | "body" + ) +} + fn is_stream_api_member(module: &str, prop: &str) -> bool { match module { "readable_stream" => matches!( @@ -2878,6 +3126,41 @@ fn is_classic_stream_method_name(prop: &str) -> bool { ) } +/// Classic Node stream (`stream` / `node:stream`) PROPERTY GETTER names — +/// the no-arg state getters that dispatch through the codegen `NativeModSig` +/// table to their `js_node_stream_method_*` FFI (mirrors the `module: "stream"` +/// getter entries in `lower_call/native_table/net_events.rs`). A bare read of +/// any name NOT in this set and NOT a `is_classic_stream_method_name` method is +/// a plain own-property GET on the heap stream object, so user-assigned fields +/// (`_.colors`, library bookkeeping) read back the stored value instead of +/// being invoked as a 0-arg native call. +fn is_classic_stream_getter_name(prop: &str) -> bool { + matches!( + prop, + "readableHighWaterMark" + | "readableLength" + | "readableObjectMode" + | "readable" + | "readableFlowing" + | "readableEnded" + | "readableEncoding" + | "readableAborted" + | "readableDidRead" + | "writableHighWaterMark" + | "writableLength" + | "writableNeedDrain" + | "writableObjectMode" + | "writable" + | "writableCorked" + | "writableEnded" + | "writableFinished" + | "closed" + | "errored" + | "allowHalfOpen" + | "destroyed" + ) +} + fn is_http_incoming_message_method_name(prop: &str) -> bool { matches!( prop, diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index 800bce3127..aeccd2f34b 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -394,6 +394,12 @@ pub fn gc_init() { // captured young values or future cache hits miss on stale addresses. gc_register_mutable_root_scanner(crate::closure::scan_singleton_closure_roots_mut); gc_register_mutable_root_scanner(crate::closure::scan_closure_dynamic_props_roots_mut); + // Generic per-handle expando properties (`blob.colors = [...]` and other + // arbitrary own props on native HANDLE values). Keys are stable small handle + // ids; only the stored VALUES are JS references that must be traced. + gc_register_mutable_root_scanner( + crate::object::handle_expando::scan_handle_expando_roots_mut, + ); // Native-module callable export singletons and process stdio stream // singletons store heap pointers in TLS caches; keep them live and rewrite // them if a copying collection moves their backing allocations. diff --git a/crates/perry-runtime/src/object/handle_expando.rs b/crates/perry-runtime/src/object/handle_expando.rs new file mode 100644 index 0000000000..09d9825173 --- /dev/null +++ b/crates/perry-runtime/src/object/handle_expando.rs @@ -0,0 +1,151 @@ +//! Generic per-handle expando property side-table. +//! +//! A native HANDLE value (Blob / fetch Response / Web-Streams reader / etc.) is +//! a NaN-boxed small integer id, NOT a heap `ObjectHeader`. The object setter +//! (`js_object_set_field_by_name`) routes a `handle.prop = v` write to +//! `js_handle_property_set_dispatch`, and a read to `js_handle_property_dispatch`. +//! Those dispatchers only know specific, typed properties (`blob.size`, +//! `response.status`, …). An ARBITRARY user-assigned own property +//! (`handle.colors = [...]`) had nowhere to land — the write was dropped and the +//! read returned `undefined`. +//! +//! In Node these objects are ordinary and freely extensible (the `debug` +//! package assigns `createDebug.colors = [...]` and later reads it back). This +//! side-table gives every handle the same arbitrary string-keyed own-property +//! storage that closures get from `CLOSURE_PROPS` (see +//! `closure/dynamic_props.rs`), modeled directly on that code. +//! +//! GC: handle ids are stable small integers that never move, so — unlike the +//! closure table — no metadata re-keying is needed. Only the stored VALUES are +//! real JS references, so the registered mutable root scanner traces them in +//! every phase (keeping e.g. a stored array and its elements alive) and rewrites +//! the stored bits when a copying collection moves the value. + +use std::collections::HashMap; +use std::sync::{Mutex, OnceLock}; + +static HANDLE_EXPANDO_PROPS: OnceLock>>> = OnceLock::new(); + +fn table() -> &'static Mutex>> { + HANDLE_EXPANDO_PROPS.get_or_init(|| Mutex::new(HashMap::new())) +} + +/// Store an arbitrary own property `name = value` on the handle `handle`. +/// Mirrors `closure_set_dynamic_prop`. The value is kept alive by the GC +/// scanner below; a write barrier publishes it for incremental/young marking. +pub fn handle_expando_set(handle: i64, name: &str, value: f64) { + if handle == 0 { + return; + } + let bits = value.to_bits(); + if let Ok(mut map) = table().lock() { + map.entry(handle) + .or_default() + .insert(name.to_string(), bits); + } + // Parent is the (non-heap) handle id, so pass 0 as the parent address — the + // scanner traces the value unconditionally, and the barrier only needs to + // mark the freshly stored child for an in-progress collection. + crate::gc::runtime_write_barrier_external_slot(0, 0, bits); +} + +/// Read back an own property previously stored via `handle_expando_set`. +/// Returns `None` when no such property exists (caller falls through to its +/// `undefined` default). Mirrors `closure_get_own_dynamic_prop`. +pub fn handle_expando_get(handle: i64, name: &str) -> Option { + if handle == 0 { + return None; + } + table() + .lock() + .ok() + .and_then(|map| map.get(&handle).and_then(|p| p.get(name).copied())) + .map(f64::from_bits) +} + +/// True when the handle has at least one user-assigned expando property. +/// (`Object.keys` / `in` support can build on this later.) +#[allow(dead_code)] +pub fn handle_expando_has_any(handle: i64) -> bool { + if handle == 0 { + return false; + } + table() + .lock() + .ok() + .map(|map| map.get(&handle).map(|p| !p.is_empty()).unwrap_or(false)) + .unwrap_or(false) +} + +/// Mutable GC root scanner for the handle expando side-table. +/// +/// Keys are stable small handle ids (never heap-moved), so this only traces the +/// stored VALUES — exactly the value half of +/// `scan_closure_dynamic_props_roots_mut`. Registered in `gc/mod.rs`. The lock +/// is released before invoking the visitor on each value (the visitor may move +/// objects and re-enter the runtime), matching the closure scanner's contract. +pub fn scan_handle_expando_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { + let owners = table() + .lock() + .ok() + .map(|map| map.keys().copied().collect::>()) + .unwrap_or_default(); + for owner in owners { + let Some(mut props) = table().lock().ok().and_then(|mut map| map.remove(&owner)) else { + continue; + }; + for bits in props.values_mut() { + let mut v = f64::from_bits(*bits); + visitor.visit_nanbox_f64_slot(&mut v); + *bits = v.to_bits(); + } + if let Ok(mut map) = table().lock() { + match map.entry(owner) { + std::collections::hash_map::Entry::Occupied(mut e) => { + // A concurrent set added entries while we held no lock; keep + // both (scanned values + any new ones). + e.get_mut().extend(props); + } + std::collections::hash_map::Entry::Vacant(e) => { + e.insert(props); + } + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn set_get_roundtrip() { + let h = 0x4_2424i64; + assert!(handle_expando_get(h, "colors").is_none()); + let v = f64::from_bits(0x7FFD_AAAA_BBBB_CCCC); + handle_expando_set(h, "colors", v); + assert_eq!(handle_expando_get(h, "colors").map(|x| x.to_bits()), Some(v.to_bits())); + assert!(handle_expando_has_any(h)); + // cleanup + if let Ok(mut map) = table().lock() { + map.remove(&h); + } + } + + #[test] + fn scanner_visits_stored_values() { + let h = 0x4_2425i64; + let v_bits = 0x7FFD_1234_5678_9ABCu64; + handle_expando_set(h, "x", f64::from_bits(v_bits)); + let mut seen: Vec = Vec::new(); + { + let mut mark = |v: f64| seen.push(v.to_bits()); + let mut visitor = crate::gc::RuntimeRootVisitor::for_copy(&mut mark); + scan_handle_expando_roots_mut(&mut visitor); + } + assert!(seen.contains(&v_bits), "scanner must trace stored value, seen={seen:x?}"); + if let Ok(mut map) = table().lock() { + map.remove(&h); + } + } +} diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index c976d2264e..37cb6a4e3a 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -37,6 +37,7 @@ mod descriptors; mod disposable_proto_thunks; pub(crate) mod exotic_expando; mod field_get_set; +pub mod handle_expando; mod field_set_by_name; mod global_fetch; mod global_this; @@ -800,8 +801,7 @@ pub(crate) fn accessor_descriptor_keys_for_obj(obj: usize) -> Vec { let mut keys = m .borrow() .keys() - .filter(|&(owner, _key)| *owner == obj) - .map(|(_owner, key)| key.clone()) + .filter_map(|(owner, key)| (*owner == obj).then(|| key.clone())) .collect::>(); keys.sort(); keys diff --git a/crates/perry-stdlib/src/common/dispatch.rs b/crates/perry-stdlib/src/common/dispatch.rs index 2087f235de..e729302b7e 100644 --- a/crates/perry-stdlib/src/common/dispatch.rs +++ b/crates/perry-stdlib/src/common/dispatch.rs @@ -2655,6 +2655,18 @@ pub unsafe extern "C" fn js_handle_property_dispatch( return crate::crypto::dispatch_cipher_property(handle, property_name); } + // Generic per-handle expando read: an arbitrary user-assigned own property + // (`handle.colors = [...]`) stored by the set-dispatch fallback below. This + // is the read half that makes native HANDLE values (Blob / fetch Response / + // Web-Streams readers) freely extensible like Node's, so the `debug` + // package's `createDebug.colors[...]` reads back the array it assigned + // instead of `undefined`. Specific typed properties were all tried above, so + // a hit here is always a genuine user expando. + if let Some(v) = perry_runtime::object::handle_expando::handle_expando_get(handle, property_name) + { + return v; + } + // Unknown handle type - return undefined f64::from_bits(0x7FFC_0000_0000_0001) } @@ -2954,8 +2966,23 @@ pub unsafe extern "C" fn js_handle_property_set_dispatch( value, ); } + return; } } + + // Generic per-handle expando store: an ARBITRARY user-assigned own property + // (`handle.colors = [...]`) that none of the typed setters above claimed. + // Native HANDLE values are ordinary, extensible objects in Node; this gives + // them the same string-keyed own-property storage closures get from + // `CLOSURE_PROPS`. The read half (`js_handle_property_dispatch`) consults + // every typed property FIRST and only falls back to this expando table, so a + // typed property name can never be shadowed by an expando copy. This is what + // makes `debug`'s `createDebug.colors = [...]` persist and read back (the + // wall: a Blob/Response-tagged `_` whose `.colors` write was silently + // dropped, so `selectColor` read `undefined`). + if !property_name.is_empty() { + perry_runtime::object::handle_expando::handle_expando_set(handle, property_name, value); + } } #[no_mangle] diff --git a/crates/perry/tests/native_instance_own_property.rs b/crates/perry/tests/native_instance_own_property.rs new file mode 100644 index 0000000000..6916472a63 --- /dev/null +++ b/crates/perry/tests/native_instance_own_property.rs @@ -0,0 +1,213 @@ +//! Regression test: arbitrary user-assigned OWN properties on a value the HIR +//! tagged as a native (classic Node stream) instance must READ/WRITE/INDEX as +//! plain object properties — while genuine native stream methods STILL +//! dispatch. +//! +//! Pre-fix wall (bundled `debug` package, pulled in by https-proxy-agent): +//! `debug`'s `selectColor` reads `_.colors` (an ARRAY field of the createDebug +//! value `_`). Because `_` was HIR-tagged a native instance, the catch-all +//! `else` arm in `crates/perry-hir/src/lower/expr_member.rs` lowered EVERY +//! non-method property read to a 0-arg `NativeMethodCall`, which codegen turned +//! into `js_native_call_method_nullsafe(inst, "colors", 0 args)` — i.e. it +//! *invoked* the stored array → `TypeError: value is not a function`. Node just +//! returns the property value. +//! +//! Fix: for `stream` / `node:stream` instances, a bare read of a name that is +//! neither a known classic stream method (`is_classic_stream_method_name`) nor +//! a known stream property getter (`is_classic_stream_getter_name`) lowers to a +//! plain `PropertyGet` (own-property storage on the heap stream object). The +//! matching `_.x = v` write already lowers to a generic `PropertySet`, so the +//! value persists and reads back. Real native methods (`.on`, `.pipe`, +//! `.destroy`, `.write`, `.end`) and getters (`.destroyed`) keep dispatching. + +use std::path::PathBuf; +use std::process::Command; + +fn perry_bin() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_perry")) +} + +fn compile_and_run(dir: &std::path::Path, source: &str) -> String { + let entry = dir.join("main.ts"); + let output = dir.join("main_bin"); + std::fs::write(&entry, source).expect("write entry"); + + let compile = Command::new(perry_bin()) + .current_dir(dir) + .arg("compile") + .arg(&entry) + .arg("-o") + .arg(&output) + .output() + .expect("run perry compile"); + assert!( + compile.status.success(), + "perry compile failed\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&compile.stdout), + String::from_utf8_lossy(&compile.stderr) + ); + + let run = Command::new(&output) + .current_dir(dir) + .output() + .expect("run compiled binary"); + assert!( + run.status.success(), + "compiled binary failed (pre-fix: 'TypeError: value is not a \ + function' when reading an own property of a native stream \ + instance)\nstatus: {:?}\nstdout:\n{}\nstderr:\n{}", + run.status, + String::from_utf8_lossy(&run.stdout), + String::from_utf8_lossy(&run.stderr) + ); + String::from_utf8_lossy(&run.stdout).into_owned() +} + +#[test] +fn native_instance_own_property_read_write_index_and_method() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + r#" +import { Readable } from "stream"; + +const _: any = Readable.from(["a"]); + +// WRITE an arbitrary own property (an array), then READ it back. Pre-fix the +// READ invoked the array as a 0-arg native method → "value is not a function". +_.colors = ["red", "green", "blue"]; +console.log("colors:", JSON.stringify(_.colors)); +console.log("len:", _.colors.length); // member read of the own array +console.log("idx:", _.colors[1]); // index into the own array + +// A scalar own property reads back its value (not invoked). +_.label = "stream-label"; +console.log("label:", _.label); + +// A genuine native stream method STILL dispatches in the same instance. +const got: number[] = []; +const r2: any = Readable.from([1, 2, 3]); +r2.on("data", (c: any) => { got.push(Number(c)); }); +r2.on("end", () => { + console.log("data:", got.join(",")); + console.log("destroyed-before:", r2.destroyed); // native getter still works + r2.destroy(); + console.log("destroyed-after:", r2.destroyed); +}); +"#, + ); + + let lines: Vec<&str> = stdout.lines().collect(); + assert!( + lines.contains(&"colors: [\"red\",\"green\",\"blue\"]"), + "own array property must read back the stored value, got:\n{stdout}" + ); + assert!( + lines.contains(&"len: 3"), + "member read on own array property must work, got:\n{stdout}" + ); + assert!( + lines.contains(&"idx: green"), + "index into own array property must work, got:\n{stdout}" + ); + assert!( + lines.contains(&"label: stream-label"), + "scalar own property must read back its value, got:\n{stdout}" + ); + assert!( + lines.contains(&"data: 1,2,3"), + "native stream method (.on) must still dispatch, got:\n{stdout}" + ); + assert!( + lines.contains(&"destroyed-before: false") && lines.contains(&"destroyed-after: true"), + "native stream getter (.destroyed) + method (.destroy) must still \ + dispatch, got:\n{stdout}" + ); +} + +/// #wall (debug `_.colors`): the same own-property bug, but for the NON-`stream` +/// native-instance modules. `debug`'s `createDebug` value is HIR-tagged as one +/// of the opaque-handle native modules (`blob` / `fetch` / `readable_stream` / +/// …), so `selectColor`'s `_.colors[...]` READ lowered to a 0-arg +/// `js_native_call_method_nullsafe(_, "colors", 0 args)` that *invoked* the +/// resolved value → `TypeError: value is not a function`. The classic-`stream` +/// guard did NOT cover these modules. +/// +/// Two-part fix: +/// 1. HIR (`lower/expr_member.rs`): an arbitrary property READ on a `blob` / +/// `fetch` instance that is not a known native data getter now lowers to a +/// plain `PropertyGet` (READS the value instead of CALLING it). +/// 2. Runtime (`object/handle_expando.rs` + the `js_handle_property_*_dispatch` +/// fallbacks): a generic per-handle expando side-table — keyed by handle +/// id, GC-traced like closures' `CLOSURE_PROPS` — so a WRITE of an arbitrary +/// own property persists and reads back, matching Node (these handles are +/// ordinary extensible objects). Typed getters (`blob.size`, `res.status`) +/// are consulted before the expando, so they always win. +/// +/// This pins: arbitrary own-property WRITE → READ → member/index round-trips on +/// a `Blob` (`module=blob`) and a fetch `Response` (`module=fetch`), with the +/// genuine native data getters STILL dispatching through their FFI. +#[test] +fn native_instance_own_property_non_stream_modules() { + let dir = tempfile::tempdir().expect("tempdir"); + let stdout = compile_and_run( + dir.path(), + r#" +// ── Blob (module=blob, class=Blob) ── +const b: any = new Blob(["hello"]); +b.colors = [6, 2, 3, 4, 5, 1]; // arbitrary own array property +console.log("blob.colors:", JSON.stringify(b.colors)); // pre-fix: invoked the array +console.log("blob.colors.len:", b.colors.length); +console.log("blob.colors.idx:", b.colors[2]); +b.label = "blob-label"; +console.log("blob.label:", b.label); +console.log("blob.size:", b.size); // native getter must STILL dispatch + +// ── fetch Response (module=fetch, class=Response) ── +const res: any = new Response("payload"); +res.colors = ["r", "g", "b"]; +console.log("res.colors:", JSON.stringify(res.colors)); +console.log("res.colors.len:", res.colors.length); +console.log("res.status:", res.status); // native getter must STILL dispatch +console.log("res.ok:", res.ok); +"#, + ); + + let lines: Vec<&str> = stdout.lines().collect(); + assert!( + lines.contains(&"blob.colors: [6,2,3,4,5,1]"), + "Blob arbitrary own array property must read back stored value, got:\n{stdout}" + ); + assert!( + lines.contains(&"blob.colors.len: 6"), + "member read on Blob own array property must work, got:\n{stdout}" + ); + assert!( + lines.contains(&"blob.colors.idx: 3"), + "index into Blob own array property must work, got:\n{stdout}" + ); + assert!( + lines.contains(&"blob.label: blob-label"), + "scalar own property on Blob must read back its value, got:\n{stdout}" + ); + assert!( + lines.contains(&"blob.size: 5"), + "native Blob getter (.size) must STILL dispatch, got:\n{stdout}" + ); + assert!( + lines.contains(&"res.colors: [\"r\",\"g\",\"b\"]"), + "Response arbitrary own property must read back stored value, got:\n{stdout}" + ); + assert!( + lines.contains(&"res.colors.len: 3"), + "member read on Response own array property must work, got:\n{stdout}" + ); + assert!( + lines.contains(&"res.status: 200"), + "native Response getter (.status) must STILL dispatch, got:\n{stdout}" + ); + assert!( + lines.contains(&"res.ok: true"), + "native Response getter (.ok) must STILL dispatch, got:\n{stdout}" + ); +} From 5209787430b1e327236da5494cff9d6f53276153 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Sat, 20 Jun 2026 01:38:09 +0200 Subject: [PATCH 2/3] style: cargo fmt --- crates/perry-hir/src/lower/expr_member.rs | 6 +----- crates/perry-runtime/src/gc/mod.rs | 4 +--- crates/perry-runtime/src/object/handle_expando.rs | 10 ++++++++-- crates/perry-runtime/src/object/mod.rs | 2 +- crates/perry-stdlib/src/common/dispatch.rs | 3 ++- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/crates/perry-hir/src/lower/expr_member.rs b/crates/perry-hir/src/lower/expr_member.rs index 1f2077c23e..4d74f48dab 100644 --- a/crates/perry-hir/src/lower/expr_member.rs +++ b/crates/perry-hir/src/lower/expr_member.rs @@ -1681,11 +1681,7 @@ fn lower_member_inner(ctx: &mut LoweringContext, member: &ast::MemberExpr) -> Re object: Box::new(object_expr), property: property_name, }); - } else if is_native_dispatch_member( - &module_name, - &class_name, - &property_name, - ) { + } else if is_native_dispatch_member(&module_name, &class_name, &property_name) { // #wall (debug `_.colors` / `_.init`): INVERTED DEFAULT. // A bare native-instance member READ now defaults to a // plain `PropertyGet` (the final `else` below). It only diff --git a/crates/perry-runtime/src/gc/mod.rs b/crates/perry-runtime/src/gc/mod.rs index aeccd2f34b..126a428e2e 100644 --- a/crates/perry-runtime/src/gc/mod.rs +++ b/crates/perry-runtime/src/gc/mod.rs @@ -397,9 +397,7 @@ pub fn gc_init() { // Generic per-handle expando properties (`blob.colors = [...]` and other // arbitrary own props on native HANDLE values). Keys are stable small handle // ids; only the stored VALUES are JS references that must be traced. - gc_register_mutable_root_scanner( - crate::object::handle_expando::scan_handle_expando_roots_mut, - ); + gc_register_mutable_root_scanner(crate::object::handle_expando::scan_handle_expando_roots_mut); // Native-module callable export singletons and process stdio stream // singletons store heap pointers in TLS caches; keep them live and rewrite // them if a copying collection moves their backing allocations. diff --git a/crates/perry-runtime/src/object/handle_expando.rs b/crates/perry-runtime/src/object/handle_expando.rs index 09d9825173..9523f82f9e 100644 --- a/crates/perry-runtime/src/object/handle_expando.rs +++ b/crates/perry-runtime/src/object/handle_expando.rs @@ -124,7 +124,10 @@ mod tests { assert!(handle_expando_get(h, "colors").is_none()); let v = f64::from_bits(0x7FFD_AAAA_BBBB_CCCC); handle_expando_set(h, "colors", v); - assert_eq!(handle_expando_get(h, "colors").map(|x| x.to_bits()), Some(v.to_bits())); + assert_eq!( + handle_expando_get(h, "colors").map(|x| x.to_bits()), + Some(v.to_bits()) + ); assert!(handle_expando_has_any(h)); // cleanup if let Ok(mut map) = table().lock() { @@ -143,7 +146,10 @@ mod tests { let mut visitor = crate::gc::RuntimeRootVisitor::for_copy(&mut mark); scan_handle_expando_roots_mut(&mut visitor); } - assert!(seen.contains(&v_bits), "scanner must trace stored value, seen={seen:x?}"); + assert!( + seen.contains(&v_bits), + "scanner must trace stored value, seen={seen:x?}" + ); if let Ok(mut map) = table().lock() { map.remove(&h); } diff --git a/crates/perry-runtime/src/object/mod.rs b/crates/perry-runtime/src/object/mod.rs index 37cb6a4e3a..1595b1fcda 100644 --- a/crates/perry-runtime/src/object/mod.rs +++ b/crates/perry-runtime/src/object/mod.rs @@ -37,10 +37,10 @@ mod descriptors; mod disposable_proto_thunks; pub(crate) mod exotic_expando; mod field_get_set; -pub mod handle_expando; mod field_set_by_name; mod global_fetch; mod global_this; +pub mod handle_expando; pub(crate) use global_this::{default_prepare_stack_trace_func_ptr, ERROR_CONSTRUCTOR_PTR}; mod global_this_tables; mod groupby; diff --git a/crates/perry-stdlib/src/common/dispatch.rs b/crates/perry-stdlib/src/common/dispatch.rs index e729302b7e..08fbde858e 100644 --- a/crates/perry-stdlib/src/common/dispatch.rs +++ b/crates/perry-stdlib/src/common/dispatch.rs @@ -2662,7 +2662,8 @@ pub unsafe extern "C" fn js_handle_property_dispatch( // package's `createDebug.colors[...]` reads back the array it assigned // instead of `undefined`. Specific typed properties were all tried above, so // a hit here is always a genuine user expando. - if let Some(v) = perry_runtime::object::handle_expando::handle_expando_get(handle, property_name) + if let Some(v) = + perry_runtime::object::handle_expando::handle_expando_get(handle, property_name) { return v; } From 13f852335655826c6915c988c03b270e62f649d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 20 Jun 2026 05:03:18 +0000 Subject: [PATCH 3/3] fix(runtime+stdlib): address CodeRabbit review on handle expando store - handle_expando: move the side-table to thread-local storage. Each runtime thread has its own arena + GC and the stored values are NaN-boxed references into that thread's arena; a process-global table let one thread's GC scanner trace/rewrite another thread's values. Thread-local aligns the table with the per-thread GC model. - handle_expando scanner: on re-insert after scanning, let re-entrant writes win on key collision (or_insert) instead of clobbering them with stale pre-scan values. - dispatch: the Fastify `user` and external-http server-response typed setters now `return` after claiming a property, so a successful typed set no longer also writes a stale expando copy. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01SQ4RdAWQhxvqyS9vWeLoTB --- .../src/object/handle_expando.rs | 91 +++++++++++-------- crates/perry-stdlib/src/common/dispatch.rs | 5 + 2 files changed, 58 insertions(+), 38 deletions(-) diff --git a/crates/perry-runtime/src/object/handle_expando.rs b/crates/perry-runtime/src/object/handle_expando.rs index 9523f82f9e..714bf26dbf 100644 --- a/crates/perry-runtime/src/object/handle_expando.rs +++ b/crates/perry-runtime/src/object/handle_expando.rs @@ -21,13 +21,20 @@ //! every phase (keeping e.g. a stored array and its elements alive) and rewrites //! the stored bits when a copying collection moves the value. +use std::cell::RefCell; use std::collections::HashMap; -use std::sync::{Mutex, OnceLock}; -static HANDLE_EXPANDO_PROPS: OnceLock>>> = OnceLock::new(); - -fn table() -> &'static Mutex>> { - HANDLE_EXPANDO_PROPS.get_or_init(|| Mutex::new(HashMap::new())) +// Per-thread storage: each runtime thread has its own arena + GC, and the +// stored values are NaN-boxed references into THAT thread's arena. A +// process-global table would let one thread's GC scanner trace/rewrite another +// thread's values across arena boundaries (cross-thread values are deep-copied, +// so a handle id never legitimately escapes its owning thread). Thread-local +// keeps the side-table aligned with the per-thread GC, matching the documented +// threading model. The mutable root scanner is registered once but reads the +// CURRENT thread's table on each GC, so each thread traces only its own values. +thread_local! { + static HANDLE_EXPANDO_PROPS: RefCell>> = + RefCell::new(HashMap::new()); } /// Store an arbitrary own property `name = value` on the handle `handle`. @@ -38,11 +45,12 @@ pub fn handle_expando_set(handle: i64, name: &str, value: f64) { return; } let bits = value.to_bits(); - if let Ok(mut map) = table().lock() { - map.entry(handle) + HANDLE_EXPANDO_PROPS.with(|cell| { + cell.borrow_mut() + .entry(handle) .or_default() .insert(name.to_string(), bits); - } + }); // Parent is the (non-heap) handle id, so pass 0 as the parent address — the // scanner traces the value unconditionally, and the barrier only needs to // mark the freshly stored child for an in-progress collection. @@ -56,10 +64,12 @@ pub fn handle_expando_get(handle: i64, name: &str) -> Option { if handle == 0 { return None; } - table() - .lock() - .ok() - .and_then(|map| map.get(&handle).and_then(|p| p.get(name).copied())) + HANDLE_EXPANDO_PROPS + .with(|cell| { + cell.borrow() + .get(&handle) + .and_then(|p| p.get(name).copied()) + }) .map(f64::from_bits) } @@ -70,28 +80,29 @@ pub fn handle_expando_has_any(handle: i64) -> bool { if handle == 0 { return false; } - table() - .lock() - .ok() - .map(|map| map.get(&handle).map(|p| !p.is_empty()).unwrap_or(false)) - .unwrap_or(false) + HANDLE_EXPANDO_PROPS.with(|cell| { + cell.borrow() + .get(&handle) + .map(|p| !p.is_empty()) + .unwrap_or(false) + }) } /// Mutable GC root scanner for the handle expando side-table. /// /// Keys are stable small handle ids (never heap-moved), so this only traces the /// stored VALUES — exactly the value half of -/// `scan_closure_dynamic_props_roots_mut`. Registered in `gc/mod.rs`. The lock -/// is released before invoking the visitor on each value (the visitor may move -/// objects and re-enter the runtime), matching the closure scanner's contract. +/// `scan_closure_dynamic_props_roots_mut`. Registered in `gc/mod.rs`. The +/// per-owner entry is removed (borrow dropped) before invoking the visitor on +/// each value, because the visitor may move objects and re-enter the runtime +/// (e.g. a `handle_expando_set` on this same thread) — matching the closure +/// scanner's contract. pub fn scan_handle_expando_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { - let owners = table() - .lock() - .ok() - .map(|map| map.keys().copied().collect::>()) - .unwrap_or_default(); + let owners: Vec = + HANDLE_EXPANDO_PROPS.with(|cell| cell.borrow().keys().copied().collect()); for owner in owners { - let Some(mut props) = table().lock().ok().and_then(|mut map| map.remove(&owner)) else { + let Some(mut props) = HANDLE_EXPANDO_PROPS.with(|cell| cell.borrow_mut().remove(&owner)) + else { continue; }; for bits in props.values_mut() { @@ -99,18 +110,22 @@ pub fn scan_handle_expando_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor visitor.visit_nanbox_f64_slot(&mut v); *bits = v.to_bits(); } - if let Ok(mut map) = table().lock() { - match map.entry(owner) { + HANDLE_EXPANDO_PROPS.with(|cell| { + match cell.borrow_mut().entry(owner) { std::collections::hash_map::Entry::Occupied(mut e) => { - // A concurrent set added entries while we held no lock; keep - // both (scanned values + any new ones). - e.get_mut().extend(props); + // A re-entrant set added/updated entries while we held no + // borrow; those newer writes must win. Only restore scanned + // keys that were not concurrently re-written. + let dst = e.get_mut(); + for (k, v) in props { + dst.entry(k).or_insert(v); + } } std::collections::hash_map::Entry::Vacant(e) => { e.insert(props); } } - } + }); } } @@ -130,9 +145,9 @@ mod tests { ); assert!(handle_expando_has_any(h)); // cleanup - if let Ok(mut map) = table().lock() { - map.remove(&h); - } + HANDLE_EXPANDO_PROPS.with(|cell| { + cell.borrow_mut().remove(&h); + }); } #[test] @@ -150,8 +165,8 @@ mod tests { seen.contains(&v_bits), "scanner must trace stored value, seen={seen:x?}" ); - if let Ok(mut map) = table().lock() { - map.remove(&h); - } + HANDLE_EXPANDO_PROPS.with(|cell| { + cell.borrow_mut().remove(&h); + }); } } diff --git a/crates/perry-stdlib/src/common/dispatch.rs b/crates/perry-stdlib/src/common/dispatch.rs index 08fbde858e..9d5d9d7c47 100644 --- a/crates/perry-stdlib/src/common/dispatch.rs +++ b/crates/perry-stdlib/src/common/dispatch.rs @@ -2875,6 +2875,9 @@ pub unsafe extern "C" fn js_handle_property_set_dispatch( if with_handle::(handle, |_| true).unwrap_or(false) { if property_name == "user" { crate::fastify::js_fastify_req_set_user_data(handle, value); + // Claimed by the typed setter — must not also fall through to the + // generic expando store below. + return; } } @@ -2902,6 +2905,8 @@ pub unsafe extern "C" fn js_handle_property_set_dispatch( value, ); } + // Claimed by the typed setter — don't also write a stale expando copy. + return; } }