From a09124d784b2f938b772dbb35a6703673469d5e4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 27 Aug 2026 20:53:39 +0200 Subject: [PATCH 1/2] fix(error): fs diagnostics become own properties of the error, not the message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An fs error's code/errno/syscall/path lived in six side tables keyed by the MESSAGE STRING's address. Two consequences, both fixed here. WRONG ERROR. Any `new Error(m)` built from the same message text picked up the unrelated fs error's fields: new Error(fsErr.message).code // ENOENT, node says undefined .syscall / .errno / .path // stat / -2 / the path The metadata belonged to the string, so anything holding that string answered to it. INVISIBLE TO REFLECTION. In node these are ordinary own properties. Served from a side table behind property getters they were absent from every enumeration path: Object.keys(e) [] -> code,errno,path,syscall hasOwnProperty('code') false -> true getOwnPropertyDescriptor undefined -> {value,writable,enumerable,configurable} JSON.stringify(e) {} -> {"errno":-2,"code":"ENOENT",...} {...e} {} -> same Any code logging or serialising a caught fs error lost its whole payload. Three sites each held a different wrong assumption about errors: * the fs builders keyed on the message string; * JSON.stringify hardcoded "{}" for GC_TYPE_ERROR — right for a plain error (message/name/stack are non-enumerable), wrong once the error has enumerable own props, so it also dropped user-assigned ones; * Object.assign/spread had no Error arm, so it copied nothing. All three now enumerate through exotic_own_keys(.., enumerable_only), the same enumeration Object.keys uses, so they cannot drift apart. ORDER. ERROR_USER_PROPS was a HashMap with an alphabetical sort_by for determinism — stable but not node's. It is insertion-ordered now, with reassignment keeping a key's original position, and the fs fields are installed in node's uvException order (errno, code, syscall, path, dest). The GC root scanner over these props moved to the ordered store. Verified against node on the claude-code host: three repro programs are byte-identical including key order. Suite 2746 passed. --- crates/perry-runtime/src/fs/errors.rs | 66 ++++++++--- crates/perry-runtime/src/json/stringify.rs | 79 +++++++++++-- .../src/node_submodules/diagnostics.rs | 105 ++++++++++++++++-- .../src/node_submodules/diagnostics_gc.rs | 2 +- crates/perry-runtime/src/object/alloc.rs | 41 +++++++ 5 files changed, 259 insertions(+), 34 deletions(-) diff --git a/crates/perry-runtime/src/fs/errors.rs b/crates/perry-runtime/src/fs/errors.rs index 417c979b40..16df1c3226 100644 --- a/crates/perry-runtime/src/fs/errors.rs +++ b/crates/perry-runtime/src/fs/errors.rs @@ -102,6 +102,54 @@ pub(crate) fn io_error_errno(err: &std::io::Error) -> i32 { } } +/// Attach Node's fs diagnostic fields to `err_ptr` as **own properties of the +/// error object**. +/// +/// These used to be registered in six side tables keyed by the MESSAGE +/// STRING's address (`register_error_code_pub` and friends), which produced two +/// defects: +/// +/// * **Wrong error.** Any `new Error(m)` built from the same message text +/// inherited the unrelated fs error's fields — `new Error(e.message).code` +/// returned `ENOENT` where node returns `undefined`, along with `.syscall`, +/// `.errno` and `.path`. Metadata belonged to the string, not the throw. +/// * **Invisible to reflection.** In node these are ordinary own properties: +/// `Object.keys(e)` is `code,errno,path,syscall`, and `JSON.stringify(e)` and +/// `{...e}` carry them. Served from a side table behind property *getters* +/// they were absent from all of it — perry returned `{}` for both, so any +/// code that logs or serialises an fs error silently lost every field. +/// +/// Keying on the error object fixes both at once, and each field then reaches +/// reflection through the same path a user assignment does. +unsafe fn attach_fs_error_props( + err_ptr: *mut crate::error::ErrorHeader, + code: &str, + errno: i32, + syscall: &str, + path: Option<&str>, + dest: Option<&str>, +) { + use crate::node_submodules::set_error_user_prop; + let owner = err_ptr as usize; + let put_str = |key: &str, s: &str| { + let boxed = js_string_from_bytes(s.as_ptr(), s.len() as u32); + set_error_user_prop(owner, key, crate::value::js_nanbox_string(boxed as i64)); + }; + // Insertion order is observable — `Object.keys`, `for…in`, `{...err}` and + // `JSON.stringify` all report it — so install these in the same order + // node's `uvException` does: errno, code, syscall, path, dest. + // `errno` is numeric in node (-2 for ENOENT), not a string. + set_error_user_prop(owner, "errno", errno as f64); + put_str("code", code); + put_str("syscall", syscall); + if let Some(p) = path { + put_str("path", p); + } + if let Some(d) = dest { + put_str("dest", d); + } +} + pub(crate) unsafe fn build_fs_error_value( err: &std::io::Error, syscall: &'static str, @@ -112,13 +160,7 @@ pub(crate) unsafe fn build_fs_error_value( let msg = format!("{}: {}, {} '{}'", code, err, syscall, path); let msg_ptr = js_string_from_bytes(msg.as_ptr(), msg.len() as u32); let err_ptr = crate::error::js_error_new_with_message(msg_ptr); - // Register code/syscall/path in the per-message side tables so the - // `.code`, `.syscall`, `.path` property getters in `field_get_set` - // surface Node-compatible values on caught errors. - crate::node_submodules::register_error_code_pub(msg_ptr, code); - crate::node_submodules::register_error_errno(msg_ptr, errno); - crate::node_submodules::register_error_syscall(msg_ptr, syscall); - crate::node_submodules::register_error_path(msg_ptr, path.to_string()); + attach_fs_error_props(err_ptr, code, errno, syscall, Some(path), None); crate::value::js_nanbox_pointer(err_ptr as i64) } @@ -136,11 +178,7 @@ pub(crate) unsafe fn build_fs_error_value_with_dest( let msg = format!("{}: {}, {} '{}' -> '{}'", code, err, syscall, path, dest); let msg_ptr = js_string_from_bytes(msg.as_ptr(), msg.len() as u32); let err_ptr = crate::error::js_error_new_with_message(msg_ptr); - crate::node_submodules::register_error_code_pub(msg_ptr, code); - crate::node_submodules::register_error_errno(msg_ptr, errno); - crate::node_submodules::register_error_syscall(msg_ptr, syscall); - crate::node_submodules::register_error_path(msg_ptr, path.to_string()); - crate::node_submodules::register_error_dest(msg_ptr, dest.to_string()); + attach_fs_error_props(err_ptr, code, errno, syscall, Some(path), Some(dest)); crate::value::js_nanbox_pointer(err_ptr as i64) } @@ -153,9 +191,7 @@ pub(crate) unsafe fn build_fs_error_value_no_path( let msg = format!("{}: {}, {}", code, err, syscall); let msg_ptr = js_string_from_bytes(msg.as_ptr(), msg.len() as u32); let err_ptr = crate::error::js_error_new_with_message(msg_ptr); - crate::node_submodules::register_error_code_pub(msg_ptr, code); - crate::node_submodules::register_error_errno(msg_ptr, errno); - crate::node_submodules::register_error_syscall(msg_ptr, syscall); + attach_fs_error_props(err_ptr, code, errno, syscall, None, None); crate::value::js_nanbox_pointer(err_ptr as i64) } diff --git a/crates/perry-runtime/src/json/stringify.rs b/crates/perry-runtime/src/json/stringify.rs index b48a9e8ee0..6718792ea6 100644 --- a/crates/perry-runtime/src/json/stringify.rs +++ b/crates/perry-runtime/src/json/stringify.rs @@ -357,6 +357,56 @@ pub(crate) unsafe fn arm_to_json_result_guard(result: f64) { } } +/// Serialize an `Error`'s own ENUMERABLE properties, the way node does. +/// +/// Errors do not have the JSObject keys/values layout, so they cannot go +/// through `stringify_object` — but they are still ordinary property bearers to +/// an observer. `exotic_own_keys(.., enumerable_only = true)` is the same +/// enumeration `Object.keys` uses on an error, so JSON output and `Object.keys` +/// cannot disagree. +/// +/// `depth` is `Some` when called from the depth-tracking variant, so nested +/// values keep the cycle/recursion budget of the caller. +unsafe fn stringify_error_own_props(ptr: *const u8, buf: &mut String, depth: Option) { + let ptr = ptr as usize; + use crate::object::exotic_expando::{exotic_get_own_property, exotic_own_keys, ExoticKind}; + let keys = exotic_own_keys(ExoticKind::Error, ptr, true); + buf.push('{'); + let mut first = true; + for key in keys { + let Some(v) = exotic_get_own_property( + ptr, + ExoticKind::Error, + &key, + f64::from_bits(bits_of_ptr(ptr)), + ) else { + continue; + }; + // `undefined` own properties are omitted from objects, per JSON.stringify. + if v.to_bits() == crate::value::TAG_UNDEFINED { + continue; + } + if !first { + buf.push(','); + } + first = false; + write_escaped_string(buf, &key); + buf.push(':'); + match depth { + Some(d) => stringify_value_depth(v, 0, buf, d + 1), + None => stringify_value(v, 0, buf), + } + } + buf.push('}'); +} + +/// NaN-box `ptr` back into the pointer value an exotic `[[Get]]` wants as its +/// `receiver` (used only to rebind `this` for an accessor property). +#[inline] +fn bits_of_ptr(ptr: usize) -> u64 { + crate::value::js_nanbox_pointer(ptr as i64).to_bits() +} + #[inline] pub(crate) unsafe fn stringify_value(value: f64, type_hint: u32, buf: &mut String) { let bits: u64 = value.to_bits(); @@ -554,15 +604,24 @@ pub(crate) unsafe fn stringify_value(value: f64, type_hint: u32, buf: &mut Strin } } crate::gc::GC_TYPE_ERROR => { - // Issue #928: Built-in Error objects (and subclasses - // like TypeError) have a dedicated `ErrorHeader` layout — - // not the JSObject keys/values layout. Routing them - // through `stringify_object` derefs garbage as a - // `keys_array` pointer and segfaults the process. - // Node's `JSON.stringify(new Error("x"))` returns "{}" - // because Error's intrinsic props (`message`, `name`, - // `stack`) are non-enumerable; mirror that. - buf.push_str("{}"); + // Issue #928: Built-in Error objects (and subclasses like + // TypeError) have a dedicated `ErrorHeader` layout — not the + // JSObject keys/values layout — so they must never reach + // `stringify_object`, which would deref garbage as a + // `keys_array` pointer and segfault. + // + // They are NOT always "{}", though. Node emits an error's own + // ENUMERABLE properties like any other object; `{}` is merely + // what a *plain* error produces, because `message`/`name`/ + // `stack` are non-enumerable: + // + // JSON.stringify(new Error("x")) -> {} + // e.foo = 1; JSON.stringify(e) -> {"foo":1} + // JSON.stringify(fsError) -> {"errno":-2,"code":"ENOENT",…} + // + // Hardcoding "{}" silently dropped every one of those, so any + // code that logs a caught error as JSON lost its whole payload. + stringify_error_own_props(ptr, buf, None); } crate::gc::GC_TYPE_MAP | crate::gc::GC_TYPE_SET => { // Map/Set have a `{size, capacity, entries/elements}` header, @@ -770,7 +829,7 @@ pub(crate) unsafe fn stringify_value_depth( } crate::gc::GC_TYPE_ERROR => { // Issue #928: see the matching branch in `stringify_value`. - buf.push_str("{}"); + stringify_error_own_props(ptr, buf, Some(depth)); } crate::gc::GC_TYPE_MAP | crate::gc::GC_TYPE_SET => { // See the matching branch in `stringify_value` — Map/Set diff --git a/crates/perry-runtime/src/node_submodules/diagnostics.rs b/crates/perry-runtime/src/node_submodules/diagnostics.rs index 87be84f73a..b58cbcd4ca 100644 --- a/crates/perry-runtime/src/node_submodules/diagnostics.rs +++ b/crates/perry-runtime/src/node_submodules/diagnostics.rs @@ -602,7 +602,19 @@ thread_local! { /// string/primitive own properties. Stale entries after a GC move of the /// error are harmless (same model as the message-keyed tables above): a /// lookup at the new address simply misses. - pub(crate) static ERROR_USER_PROPS: RefCell>> = + /// Insertion-ORDERED per error: a `Vec`, not a `HashMap`. + /// + /// ECMA-262 enumerates an object's own string keys in insertion order, and + /// that order is observable through `Object.keys`, `for…in`, `{...err}` and + /// `JSON.stringify`. Backed by a `HashMap` this list came out in hash order, + /// so `error_user_props` sorted it alphabetically to at least be + /// deterministic — which is stable but still not node's order. A caught fs + /// error serialized as `{"code":…,"errno":…,"path":…,"syscall":…}` where + /// node writes `{"errno":…,"code":…,"syscall":…,"path":…}`. + /// + /// An error carries a handful of properties, so a linear scan is cheaper + /// than hashing and the order falls out for free. + pub(crate) static ERROR_USER_PROPS: RefCell>> = RefCell::new(HashMap::new()); } @@ -629,10 +641,14 @@ pub fn set_error_user_prop(error_ptr: usize, key: &str, value: f64) { ErrUserProp::Bits(value.to_bits()) }; ERROR_USER_PROPS.with(|m| { - m.borrow_mut() - .entry(error_ptr) - .or_default() - .insert(key.to_string(), stored); + let mut map = m.borrow_mut(); + let props = map.entry(error_ptr).or_default(); + // Reassigning an existing key keeps its original position — `o.a=1; + // o.b=2; o.a=3` still enumerates `a,b` in node. + match props.iter_mut().find(|(k, _)| k == key) { + Some(slot) => slot.1 = stored, + None => props.push((key.to_string(), stored)), + } }); } @@ -645,7 +661,7 @@ pub fn error_user_prop(error_ptr: usize, key: &str) -> Option { } ERROR_USER_PROPS.with(|m| { m.borrow().get(&error_ptr).and_then(|props| { - props.get(key).map(|v| match v { + props.iter().find(|(k, _)| k == key).map(|(_, v)| match v { ErrUserProp::Str(s) => { let ptr = js_string_from_bytes(s.as_ptr(), s.len() as u32); f64::from_bits(crate::js_nanbox_string(ptr as i64).to_bits()) @@ -666,7 +682,13 @@ pub fn remove_error_user_prop(error_ptr: usize, key: &str) -> bool { ERROR_USER_PROPS.with(|m| { m.borrow_mut() .get_mut(&error_ptr) - .map(|props| props.remove(key).is_some()) + .map(|props| match props.iter().position(|(k, _)| k == key) { + Some(i) => { + props.remove(i); + true + } + None => false, + }) .unwrap_or(false) }) } @@ -701,7 +723,8 @@ pub fn error_user_props(error_ptr: usize) -> Vec<(String, f64)> { (key, materialized) }) .collect(); - props.sort_by(|a, b| a.0.cmp(&b.0)); + // No sort: the Vec is already in insertion order, which is the order + // ECMA-262 specifies and node emits. props } @@ -1998,3 +2021,69 @@ mod tests { DIAG_CHANNELS.with(|m| m.borrow_mut().clear()); } } + +#[cfg(test)] +mod error_prop_order_tests { + use super::*; + + /// Own string keys enumerate in INSERTION order, not hash or alphabetical + /// order. This is observable through `Object.keys`, `for…in`, `{...err}` + /// and `JSON.stringify`, so a caught fs error must serialize as node's + /// `{"errno":…,"code":…,"syscall":…,"path":…}`. + /// + /// The store was a `HashMap` with an alphabetical `sort_by` bolted on for + /// determinism, which is stable but wrong: it emitted `code` before + /// `errno`. Reverting to any unordered container fails this test. + #[test] + fn user_props_enumerate_in_insertion_order() { + let err = 0x4000_1000usize; + for k in ["errno", "code", "syscall", "path"] { + set_error_user_prop(err, k, 1.0); + } + let keys: Vec = error_user_props(err).into_iter().map(|(k, _)| k).collect(); + assert_eq!( + keys, + vec![ + "errno".to_string(), + "code".to_string(), + "syscall".to_string(), + "path".to_string() + ], + "fs error fields must enumerate in node's insertion order, not sorted" + ); + } + + /// Reassigning an existing key keeps its ORIGINAL position — in node, + /// `o.a=1; o.b=2; o.a=3` still enumerates `a,b`. An implementation that + /// removed-then-appended would report `b,a`. + #[test] + fn reassignment_keeps_original_position() { + let err = 0x4000_2000usize; + set_error_user_prop(err, "a", 1.0); + set_error_user_prop(err, "b", 2.0); + set_error_user_prop(err, "a", 3.0); + let keys: Vec = error_user_props(err).into_iter().map(|(k, _)| k).collect(); + assert_eq!(keys, vec!["a".to_string(), "b".to_string()]); + assert_eq!( + error_user_prop(err, "a"), + Some(3.0), + "reassignment must still update the value" + ); + } + + /// Removing a key must not disturb the order of the survivors. + #[test] + fn removal_preserves_order_of_the_rest() { + let err = 0x4000_3000usize; + for k in ["one", "two", "three"] { + set_error_user_prop(err, k, 0.0); + } + assert!(remove_error_user_prop(err, "two")); + let keys: Vec = error_user_props(err).into_iter().map(|(k, _)| k).collect(); + assert_eq!(keys, vec!["one".to_string(), "three".to_string()]); + assert!( + !remove_error_user_prop(err, "two"), + "second remove is a no-op" + ); + } +} diff --git a/crates/perry-runtime/src/node_submodules/diagnostics_gc.rs b/crates/perry-runtime/src/node_submodules/diagnostics_gc.rs index b06b7eda7b..c34e48e267 100644 --- a/crates/perry-runtime/src/node_submodules/diagnostics_gc.rs +++ b/crates/perry-runtime/src/node_submodules/diagnostics_gc.rs @@ -133,7 +133,7 @@ pub(crate) fn finalize_dead_copied_minor_from_space_errors() { pub(crate) fn scan_error_user_props_roots_mut(visitor: &mut crate::gc::RuntimeRootVisitor<'_>) { ERROR_USER_PROPS.with(|m| { for props in m.borrow_mut().values_mut() { - for v in props.values_mut() { + for (_, v) in props.iter_mut() { if let ErrUserProp::Bits(bits) = v { visitor.visit_nanbox_u64_slot(bits); } diff --git a/crates/perry-runtime/src/object/alloc.rs b/crates/perry-runtime/src/object/alloc.rs index 482e3af632..79c2d61ca9 100644 --- a/crates/perry-runtime/src/object/alloc.rs +++ b/crates/perry-runtime/src/object/alloc.rs @@ -1543,6 +1543,47 @@ pub unsafe extern "C" fn js_object_assign_one(target_f64: f64, source_f64: f64) // (Stripe's `protoExtend` does `Object.assign(Constructor, Super)` to copy a // resource class's enumerable statics like `.extend`/`.method`; without this // the call hung at `import 'stripe'`.) + // An `Error` source. Like the buffer and closure arms around it, an + // `ErrorHeader` is not the JSObject keys/values layout, so it has no + // `keys_array` for the generic path below to walk — `{...err}` and + // `Object.assign({}, err)` therefore copied NOTHING and produced `{}`. + // + // Node treats an error as an ordinary property bearer here: its own + // ENUMERABLE properties are copied, which for a caught fs error means + // `code`/`errno`/`syscall`/`path`, and for any error means whatever the + // program assigned. `message`/`name`/`stack` stay behind because they are + // non-enumerable — `exotic_own_keys(.., enumerable_only = true)` encodes + // exactly that rule, and is the same enumeration `Object.keys` and + // `JSON.stringify` use, so the three cannot disagree. + if src_raw >= 0x10000 && src_raw.is_multiple_of(8) && { + let src_gc = + (src_raw as *const u8).sub(crate::gc::GC_HEADER_SIZE) as *const crate::gc::GcHeader; + (*src_gc).obj_type == crate::gc::GC_TYPE_ERROR + } { + use crate::object::exotic_expando::{exotic_get_own_property, exotic_own_keys, ExoticKind}; + let scope = crate::gc::RuntimeHandleScope::new(); + let tgt_h = scope.root_raw_mut_ptr(target); + let receiver = crate::value::js_nanbox_pointer(src_raw as i64); + for name in exotic_own_keys(ExoticKind::Error, src_raw, true) { + let Some(value) = exotic_get_own_property(src_raw, ExoticKind::Error, &name, receiver) + else { + continue; + }; + let value_h = scope.root_nanbox_f64(value); + let key_ptr = crate::string::js_string_from_bytes(name.as_ptr(), name.len() as u32); + tgt_h.with_mut_ptr::(|tgt| { + object_assign_set_string_key( + tgt, + target_is_array, + key_ptr, + value_h.get_nanbox_f64(), + ) + }); + } + return tgt_h + .with_mut_ptr::(|tgt| crate::value::js_nanbox_pointer(tgt as i64)); + } + if crate::closure::is_closure_ptr(src_raw) { // #7200: `js_string_from_bytes` and the write funnel both allocate, and // the snapshot's VALUES are heap references held in a plain `Vec` for From 041313d676fba8b9b4fdaeedb187c5ed3e2843f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Thu, 27 Aug 2026 20:54:16 +0200 Subject: [PATCH 2/2] changelog: add fragment for #8889 --- changelog.d/8889-error-own-properties.md | 46 ++++++++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 changelog.d/8889-error-own-properties.md diff --git a/changelog.d/8889-error-own-properties.md b/changelog.d/8889-error-own-properties.md new file mode 100644 index 0000000000..fb1dc3fd66 --- /dev/null +++ b/changelog.d/8889-error-own-properties.md @@ -0,0 +1,46 @@ +Fixed an fs error's `code`/`errno`/`syscall`/`path`: they are now own properties +of the **error object** rather than entries in side tables keyed by the error's +**message string address**. + +Two defects followed from that keying, both verified against node. + +**The wrong error got the metadata.** Any `new Error(m)` built from the same +message text inherited an unrelated fs error's fields — `.code` returned +`ENOENT` where node returns `undefined`, along with `.syscall`, `.errno` and +`.path`. The metadata belonged to the string, so anything holding that string +answered to it. + +**They were invisible to reflection.** In node these are ordinary own +properties; served from a side table behind property *getters* they appeared in +none of the enumeration paths: + +| | before | after (= node) | +|---|---|---| +| `Object.keys(e)` | `[]` | `code,errno,path,syscall` | +| `hasOwnProperty('code')` | `false` | `true` | +| `getOwnPropertyDescriptor` | `undefined` | `{value,writable,enumerable,configurable}` | +| `JSON.stringify(e)` | `{}` | `{"errno":-2,"code":"ENOENT",…}` | +| `{...e}` | `{}` | same | + +Any code that logged or serialised a caught fs error lost its whole payload. + +Three sites each held a different wrong assumption about errors. The fs builders +keyed on the message string. `JSON.stringify` hardcoded `"{}"` for +`GC_TYPE_ERROR` — correct for a *plain* error, whose `message`/`name`/`stack` +are non-enumerable, but wrong once an error carries enumerable own properties, +so it also dropped **user-assigned** ones (`e.foo=1; JSON.stringify(e)` gave +`{}` where node gives `{"foo":1}`; that half is independent of fs). +`Object.assign`/spread had no Error arm and copied nothing. All three now +enumerate through `exotic_own_keys(.., enumerable_only = true)` — the same +enumeration `Object.keys` uses — so they cannot drift apart again. + +Property **order** is fixed too. `ERROR_USER_PROPS` was a `HashMap` with an +alphabetical `sort_by` bolted on for determinism: stable, but not node's. Own +string keys enumerate in insertion order per ECMA-262, and that order is +observable through all four paths above. The store is insertion-ordered now, +reassignment keeps a key's original position (`o.a=1; o.b=2; o.a=3` enumerates +`a,b`), and the fs fields install in node's `uvException` order. The GC root +scanner over these properties moved to the ordered store. + +Verified by running three repro programs against node on the same host: +byte-identical output, key order included.