fix(codegen): inline .length fast path must reject handle-band receivers (EXC_BAD_ACCESS)#5559
Merged
Merged
Conversation
The inline `.length` codegen path (property_get.rs) validated only the NaN-box TAG of the receiver (top16 == POINTER_TAG/STRING_TAG) before `inttoptr`-ing the low-48 payload and dereferencing the GC-type byte at `handle-8` and the length u32 at `handle`. A POINTER_TAG-boxed *handle-band* value — a Web Fetch handle (Headers/Request/Response/Blob, registry id in [0x40000, 0xE0000)), net/http small handle, zlib/proxy id — passes the tag check but is a registry id, NOT a heap pointer. When a value statically typed Array/String/Named actually held such a handle at runtime and reached a `.length` site, the path dereferenced the bare id at an unmapped low address and SIGSEGVd (EXC_BAD_ACCESS). lldb on a minimal repro (`(new Headers() as string[]).length`): faults on `ldurb w9, [x8, #-0x8]` with x8 = 0x40000 (FETCH_HANDLE_BAND_START) and the receiver NaN-box tag x9 = 0x7ffd... — i.e. the GC-type probe at handle-8. The exact shape of a stripped-bundle crash whose fault address was 0x40005 (the 6th fetch handle); non-deterministic because whether handle-8 is mapped (so flow reaches the length load at `handle` itself) and what GC-type byte it reads depends on page-layout/timing — sometimes routing safely to the slow path (clean rc=1) instead. Fix: gate the inline fast path on `recv_handle > HANDLE_BAND_TOP` (0xFFFFF) in addition to the tag check, mirroring js_object_get_field_ic_miss and the inline class-field guard. Handle-band receivers now route to js_value_length_f64, which classifies by registry without a raw deref. Also harden that slow path: reject the handle band (is_handle_band) before its POINTER_TAG deref so it is safe on Linux/Android/iOS too (the macOS 2 TB heap_min floor previously masked this; the 0x1000 floor elsewhere did not). Verified: minimal repro crashes rc=139 pre-fix, returns len=0 rc=0 post-fix (lldb status=0). New regression test covers handle-band ids across every sub-band. perry-runtime lib tests 1073 pass; fmt + check clean. Independent of the alias-new fix (a9e41e163), which merely advanced doctor far enough to execute the .length access on a Fetch-typed value.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR adds handle-band safety guards to the ChangesHandle-band safety fix for
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Reading
.lengthon a handle-band object — aPOINTER_TAGvalue whose payload is a small handle id rather than a heap address (Web FetchHeaders/Request/Response/Blob, net/http small handles, zlib streams, …) — segfaults (EXC_BAD_ACCESS). These handle objects don't carry a.length, but the inline.lengthfast path treated the receiver as a heap pointer and dereferenced the raw id (thehandle - 8GcHeader read /*handlelength load), hitting unmapped memory.Observed as a non-deterministic crash in code paths that touch fetch objects (e.g. reading
response.length/headers.length-style access on a handle-band receiver); non-deterministic because whether the bogus dereference faults depends on what's mapped near the small handle id.Fix
In the inline
.lengthlowering (crates/perry-codegen/src/expr/property_get.rs), gate the fast path on the receiver being a real heap pointer (> HANDLE_BAND_TOP/addr_class::HANDLE_BAND_MAX= 0xFFFFF), mirroring the guard the other property fast paths already use. A handle-band receiver falls back to the safe dynamic path (crates/perry-runtime/src/value/dynamic_object.rs), which resolves.lengthcorrectly (orundefined) without dereferencing the handle id as a pointer. Same family as the recentjs_get_iteratornear-null guard (#5549).Tests
cargo test -p perry-runtime --lib: 1072 passed. Regression coverage for.lengthon handle-band receivers (returns the right value / undefined, no deref of the handle id). Found while driving a real bundle whosedoctor/network paths read.lengthon fetch handle objects and crashed.Summary by CodeRabbit
.lengthproperty on certain value types.