fix(runtime): guard js_error_get_errors against non-error receivers (.errors corrupted regular objects) - #5543
Conversation
… of a non-error object Codegen lowers EVERY `obj.errors` property read (receiver frequently `any`) to the native `js_error_get_errors` accessor and then unconditionally OR-s POINTER_TAG onto the result. The accessor blindly read `ErrorHeader.errors` at byte offset +48, which is only valid for a genuine native error. Applied to a regular user object, +48 is an unrelated property slot. Observed in a deep async-dispatched config-resolution `for…of` over `obj.errors`: the slot held NaN-boxed `undefined` (0x7FFC_0000_0000_0001); codegen OR-ed POINTER_TAG to produce 0x7FFD_0000_0000_0001 — a handle-band id (raw=1), not a heap array. `js_get_iterator` then treated it as an iterator whose `.next()` routed to the small-handle dispatcher (no `next`) → undefined → 'TypeError: Iterator result is not an object'. Fix: validate the receiver is a native error (object_type == OBJECT_TYPE_ERROR, heap-plausible address) before reading the fixed slot. For any other receiver, resolve `errors` as an ordinary own property and hand back its clean pointer (or null, which the caller's re-tag turns into a not-iterable null receiver) — matching what a generic dynamic property read would have produced. Instruction-level diagnosis (lldb, debug bundle): the corrupt 0x7FFD_0000_0000_0001 entered js_get_iterator from cli_ts__li5 at the `bl js_error_get_errors` → `orr POINTER_TAG` site; the receiver object had object_type=0x1 (OBJECT_TYPE_REGULAR) with +48 = 0x7FFC_0000_0000_0001. Verified end-to-end: relinking the bundle with the fixed runtime advances `agents` past this wall (to an unrelated downstream error). Tests: 2 new unit tests (regular object resolves real .errors property / null when absent / rejects handle-band ptr; native AggregateError still uses the fixed slot). perry-runtime lib: 1071 passed.
📝 WalkthroughWalkthrough
Changesjs_error_get_errors Safety Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/perry-runtime/src/error.rs (2)
1374-1378: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueConsider interning the
"errors"key string.
js_string_from_bytesallocates a fresh string on every non-error call. Since this is a hot path when iterating.errorson user objects, you could intern or cache the key to avoid repeated allocations. That said, this is a micro-optimization and the current approach is functionally correct.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/error.rs` around lines 1374 - 1378, The `js_string_from_bytes` call for the "errors" key allocates a fresh string on every invocation in this hot path, which is inefficient. To fix this, intern or cache the "errors" key string at module level or as a static/lazy-static variable so that the same allocated string is reused across multiple calls to js_object_get_field_by_name. Replace the inline js_string_from_bytes call with a reference to the cached/interned key to eliminate repeated allocations.
1619-1655: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueTests placed in
tostring_testsmodule.These tests cover
js_error_get_errorsbehavior, notto_stringfunctionality. Consider moving them to a dedicatedget_errors_testsmodule or renaming the containing module to something more general likeerror_accessor_tests. This is a minor organizational nit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/perry-runtime/src/error.rs` around lines 1619 - 1655, The test function get_errors_on_regular_object_reads_real_property_not_fixed_slot is placed in the tostring_tests module, but it tests js_error_get_errors functionality rather than to_string behavior, causing organizational misalignment. Either move this test to a dedicated get_errors_tests module or rename the containing tostring_tests module to something more general like error_accessor_tests that accurately reflects its broader test coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/perry-runtime/src/error.rs`:
- Around line 1360-1366: Update the comment near the is_plausible_heap_addr call
to accurately reflect that the validation logic differs from js_error_is_error,
not matches it. The comment should note that while js_error_is_error uses only
is_valid_obj_ptr for validation, this code uses the stricter
is_plausible_heap_addr which includes an additional is_above_handle_band check,
making it more defensive. Clarify in the comment that this is intentional and
more restrictive validation than what is_valid_obj_ptr alone provides.
---
Nitpick comments:
In `@crates/perry-runtime/src/error.rs`:
- Around line 1374-1378: The `js_string_from_bytes` call for the "errors" key
allocates a fresh string on every invocation in this hot path, which is
inefficient. To fix this, intern or cache the "errors" key string at module
level or as a static/lazy-static variable so that the same allocated string is
reused across multiple calls to js_object_get_field_by_name. Replace the inline
js_string_from_bytes call with a reference to the cached/interned key to
eliminate repeated allocations.
- Around line 1619-1655: The test function
get_errors_on_regular_object_reads_real_property_not_fixed_slot is placed in the
tostring_tests module, but it tests js_error_get_errors functionality rather
than to_string behavior, causing organizational misalignment. Either move this
test to a dedicated get_errors_tests module or rename the containing
tostring_tests module to something more general like error_accessor_tests that
accurately reflects its broader test coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ebf815a4-519d-484a-bf01-ca32f26cd2d3
📒 Files selected for processing (1)
crates/perry-runtime/src/error.rs
| if !crate::value::addr_class::is_plausible_heap_addr(addr) { | ||
| return std::ptr::null_mut(); | ||
| } | ||
| // Native error objects carry `object_type == OBJECT_TYPE_ERROR` in | ||
| // their first u32; only those have the `errors` field at a fixed | ||
| // offset. (Matches the validation in `js_error_is_error`.) | ||
| let object_type = std::ptr::read(error as *const u32); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if is_plausible_heap_addr and is_valid_obj_ptr have the same semantics
# or if one is a superset of the other
# Find definitions of both functions
echo "=== is_plausible_heap_addr definition ==="
ast-grep --pattern 'fn is_plausible_heap_addr($$$) { $$$ }'
echo ""
echo "=== is_valid_obj_ptr definition ==="
ast-grep --pattern 'fn is_valid_obj_ptr($$$) { $$$ }'
echo ""
echo "=== All usages of is_plausible_heap_addr ==="
rg -n 'is_plausible_heap_addr' --type rust
echo ""
echo "=== All usages of is_valid_obj_ptr ==="
rg -n 'is_valid_obj_ptr' --type rustRepository: PerryTS/perry
Length of output: 26574
🏁 Script executed:
# Get the function definitions for is_plausible_heap_addr and is_valid_obj_ptr
sed -n '140,200p' crates/perry-runtime/src/value/addr_class.rsRepository: PerryTS/perry
Length of output: 2576
🏁 Script executed:
# Check js_error_is_error implementation
sed -n '729,750p' crates/perry-runtime/src/error.rsRepository: PerryTS/perry
Length of output: 881
🏁 Script executed:
# Check is_above_handle_band definition and js_error_get_errors signature
rg -n 'fn is_above_handle_band' crates/perry-runtime/src/value/addr_class.rs -A 5Repository: PerryTS/perry
Length of output: 312
🏁 Script executed:
# Get HANDLE_BAND_MAX value to understand the range
rg -n 'HANDLE_BAND_MAX' crates/perry-runtime/src/value/addr_class.rs -B 2Repository: PerryTS/perry
Length of output: 1999
🏁 Script executed:
# Get full js_error_get_errors function to see complete context
sed -n '1332,1385p' crates/perry-runtime/src/error.rsRepository: PerryTS/perry
Length of output: 2904
Update comment at line 1365: validation functions differ, not equivalent.
The comment claims this code "Matches the validation in js_error_is_error", but js_error_is_error (line 736) uses only is_valid_obj_ptr, while this code uses is_plausible_heap_addr. These have different semantics: is_plausible_heap_addr adds is_above_handle_band(addr) (addr >= 0x100000) on top of the heap-range check, making it stricter. Addresses in the handle band ([0x1000, 0x100000) on Linux) would pass is_valid_obj_ptr but fail is_plausible_heap_addr. The more defensive check here is sound, but the comment should reflect that the validations differ, not match.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/perry-runtime/src/error.rs` around lines 1360 - 1366, Update the
comment near the is_plausible_heap_addr call to accurately reflect that the
validation logic differs from js_error_is_error, not matches it. The comment
should note that while js_error_is_error uses only is_valid_obj_ptr for
validation, this code uses the stricter is_plausible_heap_addr which includes an
additional is_above_handle_band check, making it more defensive. Clarify in the
comment that this is intentional and more restrictive validation than what
is_valid_obj_ptr alone provides.
Problem
for (const x of obj.errors)(and anyobj.errorsread) corrupted its value whenobjis a regular object, not a native error — surfacing downstream asTypeError: Iterator result is not an object.Root cause (pinned at the instruction level)
obj.errorsproperty read (receiver typicallyany) to the nativejs_error_get_errorsaccessor, then unconditionally OR-sPOINTER_TAGonto the result (for theAggregateError.errorsarray fast path).js_error_get_errors(crates/perry-runtime/src/error.rs) blindly read(*ErrorHeader).errorsat the fixed byte offset +48 — only valid for a genuine native error.undefined(0x7FFC_0000_0000_0001). OR-ingPOINTER_TAGyields0x7FFD_0000_0000_0001— a pointer-tagged value with payload1.js_get_iteratorthen treats raw1as a handle-band iterator;.next()finds no dispatcher → returns undefined → "Iterator result is not an object".(Confirmed in lldb: the corrupt receiver entered
js_get_iteratorstraight from thebl js_error_get_errors→orr POINTER_TAGsite; the receiver'sobject_typewasOBJECT_TYPE_REGULAR, and+48held0x7FFC…0001.)Fix
js_error_get_errorsnow validates the receiver is a heap-plausible native error (object_type == OBJECT_TYPE_ERROR) before reading the +48 slot. For any other receiver it resolveserrorsas an ordinary own property and returns its real value (a clean array pointer, or null when absent →for…ofover null reports "not iterable", as a generic read would). NativeAggregateErrorstill uses the fixed slot.Tests
cargo test -p perry-runtime --lib: 1071 passed (+2 new: a regular object resolves its real.errorsarray / null when absent / rejects a handle-band pointer; native AggregateError unchanged). Verified against a real large bundle: subcommands that previously crashed here now advance past it.Summary by CodeRabbit