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. 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