fix(runtime): guard URLSearchParams shape probe against non-object receivers (Date.toString segv)#5997
Conversation
|
Warning Review limit reached
Next review available in: 31 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe ChangesShape Detection Hardening
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/url/search_params.rs`:
- Around line 830-837: Update shape_is_url_search_params to reject non-object
receivers before any GC-header dereference: the current guard only checks the
object pointer against GC_HEADER_SIZE, but this probe can be called with
type-erased receivers like registered buffers. Add the same raw-receiver
validation used elsewhere (the raw < 0x10000 and is_registered_buffer(raw)
check) before computing gc_header or reading obj_type, so non-object cells are
filtered out safely.
🪄 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: ab346e3c-bfb4-40e2-8891-40185fdc3847
📒 Files selected for processing (1)
crates/perry-runtime/src/url/search_params.rs
…ceivers PR #5964 made native URLSearchParams methods reachable through dynamic access by probing the receiver in js_native_call_method for the method names append/set/get/has/delete/toString. That probe masks the receiver bits into an *ObjectHeader and calls shape_is_url_search_params, which reads class_id / keys_array / length at ObjectHeader offsets. But the receiver may be any pointer-tagged value, not an ordinary object. A Date value is a pointer to a GC_TYPE_DATE_CELL, whose bytes at those offsets are garbage. Reading keys_array as a pointer and dereferencing its length segfaulted -- so any two Date.toString() calls (the probe fires on toString) crashed: language/expressions/addition/S11.6.1_A2.2_T2.js (the test does date.toString() + date.toString()). Gate shape_is_url_search_params on the GC header type: only proceed when the allocation is GC_TYPE_OBJECT. A Date cell (GC_TYPE_DATE_CELL), array, buffer, closure, etc. now returns false immediately instead of reading ObjectHeader fields off a foreign layout. This protects every caller of the shared probe, not just the new toString arm. Verified on an internal Linux sweep host: the addition test passes, Date.toString()+Date.toString() no longer segfaults, and URLSearchParams dynamic get/toString/size still work. The addition slice is clean; the remaining Date runtime-fails are pre-existing unrelated gaps. Refs #5961
c2784c9 to
0963635
Compare
Problem
language/expressions/addition/S11.6.1_A2.2_T2.jsregressed to a segfault (SIGSEGV, "no output") after #5964.#5964 made native
URLSearchParamsmethods reachable through dynamic access by probing the receiver injs_native_call_methodforappend/set/get/has/delete/toString. The probe masks the receiver bits into an*ObjectHeaderand callsshape_is_url_search_params, which readsclass_id/keys_array/lengthatObjectHeaderoffsets.But the receiver can be any pointer-tagged value. A
Datevalue is a pointer to aGC_TYPE_DATE_CELL, whose bytes at those offsets are unrelated. Readingkeys_arrayas a pointer and dereferencing itslengthfaulted — so the test'sdate.toString() + date.toString()(twotoStringcalls, each firing the probe) crashed.Fix
Gate
shape_is_url_search_paramson the GC header type: proceed only when the allocation isGC_TYPE_OBJECT. A Date cell, array, buffer, closure, etc. now returnsfalseimmediately instead of readingObjectHeaderfields off a foreign layout. This protects every caller of the shared probe, not just the newtoStringarm — mirroring theis_plain_matcher_objectGC-header check already used elsewhere.Verification
Built on an internal Linux sweep host:
language/expressions/addition/S11.6.1_A2.2_T2.jspasses (was SIGSEGV).date.toString() + date.toString()no longer segfaults.URLSearchParamsdynamicget/toString/sizestill work (no regression to fix(runtime): #5961 — URLSearchParams methods reachable through dynamic access #5964's fix).language/expressions/additionslice is clean; remainingbuilt-ins/Dateruntime-fails are pre-existing unrelated gaps (toTemporalInstant, MakeTime precision, etc.).Refs #5961. Code-only (no version/changelog).
Summary by CodeRabbit