fix(gc): JSON.parse read its input after the source string moved - #7373
Merged
Conversation
Both js_json_parse and js_json_parse_result derived a byte slice from the source StringHeader, then called gc_check_trigger() -- a deliberate collection point, there to shed parse-churn garbage between iterations -- and only THEN pushed the string's GC root and suppressed collection. An evacuating minor at that trigger relocates the string, and the parser reads the pre-collection address for the whole parse. Ordering is the entire fix. I tried re-deriving the slice after the suppression first and it changed nothing, because the ROOT was also pushed after the collection: rooting an address the collector has already moved away from means reading it back returns the same stale pointer. Rooting before the trigger is what makes the re-read work. Two separate copies of this pattern existed; the backtrace named js_json_parse and I patched js_json_parse_result first, which is why the first attempt measured 0/8. Both are fixed. Invisible from output -- evacuation copies rather than zeroes, so the stale address still held the right bytes and every test printed the correct answer. PERRY_GC_PROTECT_FROMSPACE faults on the first peek(). 8/8 cluster tests now clean (6/6 faults before, 0/6 after), 7/8 byte-identical to Node with the 8th already differing on main, 55/55 json unit tests pass. Closes the largest cluster in #7341.
📝 WalkthroughWalkthroughThe JSON parser now roots source strings before garbage collection and re-derives their byte slices afterward. The change applies to ChangesJSON parser GC safety
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/parse_api.rs`:
- Around line 289-305: Move creation of the source-string root before the first
collection point, including gc_collect_pending_suppressed_parse(), and carry
that rooted handle through the tape branch. After every possible collection,
re-derive text_ptr and bytes from the rooted value before parsing, restore/pop
the root on every return path, and add a from-space-quarantine test covering
pending collection with PERRY_JSON_TAPE=1.
- Around line 152-157: The bytes slice derived from text_root at lines 152-157
becomes invalid after the post-parse GC at lines 161-163 relocates the source
string, and similarly at lines 315-320 the parser state and bytes become stale
after GC at lines 329-331. At the anchor site (lines 152-157), move the
evaluation of any null or error input state that depends on bytes to after the
GC sequence completes, or re-derive the bytes from text_root after the GC before
they are used in null/error handling. At the sibling site (lines 315-320),
evaluate parser.has_trailing_content() before the GC sequence begins (before
lines 329-331), and re-derive or copy any bytes needed by null or error paths
after the GC but before text_root is restored. Ensure all GC-managed values
remain rooted and all string byte views are re-derived after moving GC rather
than held across the collection point.
🪄 Autofix
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: 8281cfe4-d4bf-46e4-9e49-a7adc14740b2
📒 Files selected for processing (2)
changelog.d/7373-json-parse-stale-input.mdcrates/perry-runtime/src/json/parse_api.rs
Comment on lines
+152
to
+157
| let bytes = { | ||
| let moved = crate::json::parse_root_get(text_root); | ||
| let hdr = moved.as_string_ptr(); | ||
| let data_ptr = (hdr as *const u8).add(std::mem::size_of::<StringHeader>()); | ||
| std::slice::from_raw_parts(data_ptr, len) | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Do not retain parser input across post-parse GC.
The post-parse GC sequence can relocate the source string. Updating text_root does not update bytes or DirectParser. Both functions restore text_root before later input reads.
crates/perry-runtime/src/json/parse_api.rs#L152-L157: Evaluate or copy all null/error input state while GC is suppressed, or re-derive it after Lines 161-163 and before restoringtext_root.crates/perry-runtime/src/json/parse_api.rs#L315-L320: Evaluateparser.has_trailing_content()before Lines 329-331. Re-derive or copy bytes needed by null/error handling before restoringtext_root.
When parse-boundary collection runs, normal trailing-content checks and null/error paths can read retired from-space.
As per coding guidelines, GC-managed values must remain rooted across every possible collection point. Based on learnings, source-string byte views are invalid after moving GC until they are re-derived.
📍 Affects 1 file
crates/perry-runtime/src/json/parse_api.rs#L152-L157(this comment)crates/perry-runtime/src/json/parse_api.rs#L315-L320
🤖 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/parse_api.rs` around lines 152 - 157, The bytes
slice derived from text_root at lines 152-157 becomes invalid after the
post-parse GC at lines 161-163 relocates the source string, and similarly at
lines 315-320 the parser state and bytes become stale after GC at lines 329-331.
At the anchor site (lines 152-157), move the evaluation of any null or error
input state that depends on bytes to after the GC sequence completes, or
re-derive the bytes from text_root after the GC before they are used in
null/error handling. At the sibling site (lines 315-320), evaluate
parser.has_trailing_content() before the GC sequence begins (before lines
329-331), and re-derive or copy any bytes needed by null or error paths after
the GC but before text_root is restored. Ensure all GC-managed values remain
rooted and all string byte views are re-derived after moving GC rather than held
across the collection point.
Sources: Coding guidelines, Learnings
Comment on lines
+289
to
+305
| // #7341: root the source string BEFORE `gc_check_trigger`, and re-derive | ||
| // the input slice from the rooted value afterwards. | ||
| // | ||
| // The `gc_check_trigger()` immediately below is deliberate — see the | ||
| // comment above, it is what keeps parse-churn garbage shedding between | ||
| // iterations — but it is a COLLECTION POINT, and both `bytes` and | ||
| // `text_ptr` were derived above it. An evacuating minor there moves the | ||
| // source string, and the parser then reads the pre-collection address for | ||
| // the entire parse; the from-space quarantine reports it as a fault at | ||
| // `parse_value + 36`, on the very first `peek()`. | ||
| // | ||
| // Pushing the root first is what makes the re-read work. The old order | ||
| // pushed it AFTER the trigger, which roots an address the collector has | ||
| // already moved away from — so re-deriving from that slot returns the same | ||
| // stale pointer and fixes nothing. Rooting first means the collector | ||
| // rewrites the slot, and the re-read yields the post-move payload. | ||
| let text_root = parse_root_push(JSValue::string_ptr(text_ptr as *mut StringHeader)); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
Root the input before the first collection point.
Line 305 runs after gc_collect_pending_suppressed_parse() at Line 208. That helper can call gc_check_trigger() and relocate the string. If it collects, the root records a from-space address and Line 316 reloads that stale address.
The tape branch at Lines 268-270 also runs before Line 305. Its helper has collection points before its local root. Therefore forced tape mode can still consume stale text_ptr and bytes.
Create the root before Line 208. Carry a rooted handle through the tape branch. Re-derive the pointer and byte slice after each collection. Restore the root on every return path. Add a from-space-quarantine test that exercises pending collection and PERRY_JSON_TAPE=1.
As per coding guidelines, GC-managed values must remain rooted across every possible collection point. Based on learnings, a movable source-string byte view must be reloaded after a GC.
🤖 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/parse_api.rs` around lines 289 - 305, Move
creation of the source-string root before the first collection point, including
gc_collect_pending_suppressed_parse(), and carry that rooted handle through the
tape branch. After every possible collection, re-derive text_ptr and bytes from
the rooted value before parsing, restore/pop the root on every return path, and
add a from-space-quarantine test covering pending collection with
PERRY_JSON_TAPE=1.
Sources: Coding guidelines, Learnings
proggeramlug
added a commit
that referenced
this pull request
Aug 4, 2026
…r it was written (#7378) #7341's quarantine produced 31 real stale-pointer bugs; four were fixed (#7373-#7376). None existed when the RFC's 'would it have caught the real bugs' table was written, so they are the strongest available calibration. It would have caught ONE of the four. #7375 (await polling a moved promise) is squarely in scope and would have been caught completely -- notably it had survived a comment explicitly reasoning about the surrounding hazard, which is the RFC's own central argument. The other three lived in perry-runtime, which this RFC does not govern. Recording that honestly rather than only the win, because the ratio is the useful part: it says where the remaining risk lives. Three of four were layer 3, where RuntimeHandleScope exists (675 uses) but is optional. The sharper finding is that all four were the SAME defect shape -- ordering a root relative to a collection point, never a missing root. That is exactly what a Raw that dies at the next &mut emit enforces, so the design generalises; the open question is whether layer 3 needs the same discipline rather than whether codegen does. Also notes the sample is favourable: the eight catches left open in #7341 are caller-side, and Raw<'e> does not cross a function boundary either. No code change -- the RFC says step 1 wants a quiet tree, and expr/ is under edit by #7375. Co-authored-by: Ralph Küpper <ralph@skelpo.com>
This was referenced Aug 4, 2026
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.
Closes the largest cluster in #7341 — 8 of 31 quarantine catches.
The bug
Both
js_json_parseandjs_json_parse_resultdid this:The
gc_check_trigger()is intentional — it sheds parse-churn garbage between iterations — but it is a collection point, and both the slice and the root came after it.Ordering is the entire fix
I tried re-deriving the slice after the suppression first, and it measured 0/8. Pushing the root after the collection roots an address the collector has already moved away from, so reading it back returns the same stale pointer. The root has to precede the trigger; then the re-read yields the post-move payload.
Two copies of the pattern exist. The backtrace names
js_json_parse; I patchedjs_json_parse_resultfirst, which is why the first attempt changed nothing. Both are fixed.Verification
8/8 cluster tests go from 6/6 faults to 0/6. 7/8 byte-identical to Node — the 8th (
test_gap_6375_legacy_url_parse_format) was alreadydiffon main before this change. 55/55 JSON unit tests pass.Invisible from output on purpose: evacuation copies rather than zeroes, so the stale address still held the right bytes and every test printed the correct answer.
PERRY_GC_PROTECT_FROMSPACE=1faults on the firstpeek().Summary by CodeRabbit