fix(json): JSON.stringify segfaulted on a non-ObjectHeader pointer#6334
Open
proggeramlug wants to merge 2 commits into
Open
fix(json): JSON.stringify segfaulted on a non-ObjectHeader pointer#6334proggeramlug wants to merge 2 commits into
proggeramlug wants to merge 2 commits into
Conversation
`JSON.stringify` could SIGSEGV. `is_object_pointer` checked that the value's
pointer was not a handle-band id, then read `(*obj).keys_array` and validated
*that* slot with only an alignment/magnitude heuristic:
top16 in {0,1} && p > 0x10000 && (p & 7) == 0
A garbage word like `0x223af100` satisfies all three, so the following
`(*keys_arr).length` load dereferenced unmapped memory and faulted.
The deeper problem is that being GC-tracked only proves the *allocation* is real —
not that it has an `ObjectHeader` layout. A Promise / WeakMap / ArrayBuffer reaches
the object walkers anyway (e.g. through a static `TYPE_OBJECT` hint from codegen),
and then that slot is some unrelated internal field being read as a pointer. The
same held for `object_has_no_own_keys` and for the pretty printer's walkers, which
loaded `field_count` / `keys_array` with no gate at all.
- `ptr_is_tracked_heap_object` (dereference-free: page-map + malloc registry) now
gates the object pointer itself, and — new — the keys array it points to.
- `object_keys_array_checked` is the single accessor every object walker uses; a
slot that isn't a tracked allocation yields `{}` instead of a fault.
Fixing the walk exposed four value-level divergences from Node on the way, all in
the pretty (`space`-argument) path, which had drifted from the plain one:
- a function inside an array serialized as its own pointer bits reinterpreted as a
string (`[function(){}]` → `[""]`, and for most closures a fault). Node emits
`null`; every other array path already did.
- `JSON.stringify(new Date(), null, 2)` walked the `DateCell` as a plain object and
produced `""` instead of the ISO string — silently corrupting any indented JSON
file containing a date. The plain path has always had the `toJSON` arm.
- a `RegExp` emitted `{"field0":null}` (its `RegExpHeader` slots read as fields);
Node emits `{}`.
- a `Promise` fell through to the `StringHeader` fallback and emitted `""`; Node
emits `{}`. Detected by GC type alongside Map/Set/Error.
Validation: a node-vs-perry differential over a value matrix — functions (bare, in
arrays, as properties), dates, regexps, Map/Set/Error, Promise, WeakMap/WeakSet/
WeakRef, sync + async generators, buffers, typed arrays, boxed primitives, sparse
arrays, replacer functions, key whitelists, deep nesting, circular detection — is
byte-identical to Node in both the plain and pretty forms, where 8 cases previously
diverged and several faulted. perry-runtime: 1264 passed / 0 failed.
On a large esbuild-bundled CLI app this removes a reproducible SIGSEGV: 42/42 runs
clean, from 5/25 crashing.
Known gaps left for follow-ups (pre-existing, none of them faults): `ArrayBuffer`/
`DataView` serialize as a Buffer rather than `{}`; a `Proxy` yields `null` rather
than its target's properties; `new URL(...)` throws "Converting circular structure
to JSON" instead of emitting `href`; `Headers`/`TextEncoder` yield `null`, and
`AbortController`/`AbortSignal` expose internals, where Node emits `{}`.
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughJSON serialization now validates tracked pointers and object key arrays before dereferencing, handles Promise, RegExp, and Date values explicitly, and serializes invalid or non-serializable array elements safely. ChangesJSON stringify safety
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/json/stringify.rs`:
- Around line 1983-1989: Update the array-element match in stringify_value_depth
to check regex_header_has_magic before dispatching on gc_obj_type, emitting "{}"
for RegExp elements such as /re/. Align the GC_TYPE_PROMISE arm with the
surrounding match arms so cargo fmt --check passes.
🪄 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: 28002b57-86e0-4ccf-be7c-a0dadbd4d473
📒 Files selected for processing (2)
crates/perry-runtime/src/json/replacer.rscrates/perry-runtime/src/json/stringify.rs
Comment on lines
+1983
to
+1989
| // A Promise has no enumerable own properties — Node emits "{}". Its | ||
| // `PromiseHeader` is not the JSObject keys/values layout, so falling | ||
| // through to the structural heuristics below read its slots as a | ||
| // StringHeader and emitted `""`. | ||
| crate::gc::GC_TYPE_PROMISE => { | ||
| buf.push_str("{}"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Missing RegExp arm (parity gap with stringify_value_depth) plus indentation drift.
Two issues in this array-element match:
- Like the top-level
stringify_valuegap above, array elements here dispatch ongc_obj_type(elem_ptr)without a priorregex_header_has_magiccheck, so a RegExp element (e.g.JSON.stringify([/re/])) falls into theGC_TYPE_OBJECTarm at line 1968 and risks misreadingRegExpHeaderasObjectHeaderinstead of emitting{}. - The new
GC_TYPE_PROMISEarm is indented 12 spaces while the surroundingGC_TYPE_MAP | GC_TYPE_SETarm and the_ =>catch-all use 16 spaces. Based on learnings, CI only runscargo fmt --check(no clippy) for this repo, so this indentation mismatch would fail that check.
🐛 Proposed fix — add RegExp arm + fix indentation
+ if crate::regex::regex_header_has_magic(elem_ptr as *const crate::regex::RegExpHeader) {
+ buf.push_str("{}");
+ continue;
+ }
match gc_obj_type(elem_ptr) {
crate::gc::GC_TYPE_OBJECT => stringify_object_inner(elem_ptr, buf, depth),
crate::gc::GC_TYPE_ARRAY => stringify_array_depth(elem_ptr, buf, depth),
@@
crate::gc::GC_TYPE_MAP | crate::gc::GC_TYPE_SET => {
buf.push_str("{}");
}
- // A Promise has no enumerable own properties — Node emits "{}". Its
- // `PromiseHeader` is not the JSObject keys/values layout, so falling
- // through to the structural heuristics below read its slots as a
- // StringHeader and emitted `""`.
- crate::gc::GC_TYPE_PROMISE => {
- buf.push_str("{}");
- }
+ crate::gc::GC_TYPE_PROMISE => {
+ // A Promise has no enumerable own properties — Node emits "{}".
+ buf.push_str("{}");
+ }
_ => {🤖 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/json/stringify.rs` around lines 1983 - 1989, Update
the array-element match in stringify_value_depth to check regex_header_has_magic
before dispatching on gc_obj_type, emitting "{}" for RegExp elements such as
/re/. Align the GC_TYPE_PROMISE arm with the surrounding match arms so cargo fmt
--check passes.
Source: Learnings
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.
JSON.stringifycould SIGSEGV.is_object_pointerchecked that the value's pointer was not a handle-band id, then read(*obj).keys_arrayand validated that slot with only an alignment/magnitude heuristic:A garbage word like
0x223af100satisfies all three, so the following(*keys_arr).lengthload dereferenced unmapped memory and faulted.The deeper problem is that being GC-tracked only proves the allocation is real — not that it has an
ObjectHeaderlayout. A Promise / WeakMap / ArrayBuffer reaches the object walkers anyway (e.g. through a staticTYPE_OBJECThint from codegen), and then that slot is some unrelated internal field being read as a pointer. The same held forobject_has_no_own_keysand for the pretty printer's walkers, which loadedfield_count/keys_arraywith no gate at all.ptr_is_tracked_heap_object(dereference-free: page-map + malloc registry) now gates the object pointer itself, and — new — the keys array it points to.object_keys_array_checkedis the single accessor every object walker uses; a slot that isn't a tracked allocation yields{}instead of a fault.Value-level divergences this exposed
All in the pretty (
space-argument) path, which had drifted from the plain one:[function(){}]→[""], and for most closures a fault). Node emitsnull; every other array path already did.JSON.stringify(new Date(), null, 2)walked theDateCellas a plain object and produced""instead of the ISO string — silently corrupting any indented JSON file containing a date. The plain path has always had thetoJSONarm.RegExpemitted{"field0":null}(itsRegExpHeaderslots read as fields); Node emits{}.Promisefell through to theStringHeaderfallback and emitted""; Node emits{}. Detected by GC type alongside Map/Set/Error.Validation
A node-vs-perry differential over a value matrix — functions (bare, in arrays, as properties), dates, regexps, Map/Set/Error, Promise, WeakMap/WeakSet/WeakRef, sync + async generators, buffers, typed arrays, boxed primitives, sparse arrays, replacer functions, key whitelists, deep nesting, circular detection — is byte-identical to Node in both the plain and pretty forms, where 8 cases previously diverged and several faulted.
perry-runtime: 1264 passed / 0 failed (also with--test-threads=1).On a large esbuild-bundled CLI app this removes a reproducible SIGSEGV: 42/42 runs clean, from 5/25 crashing.
Known gaps left for follow-ups
Pre-existing, and none of them faults:
ArrayBuffer/DataViewserialize as a Buffer rather than{}; aProxyyieldsnullrather than its target's properties;new URL(...)throws "Converting circular structure to JSON" instead of emittinghref;Headers/TextEncoderyieldnull, andAbortController/AbortSignalexpose internals, where Node emits{}.Summary by CodeRabbit
keys_arraylayouts before reading headers.JSON.stringifybehavior for edge GC types:PromiseandRegExpnow serialize as{};DateusestoJSON()semantics."null"and array elements that are sparse/undefined/functions serialize asnull.