From 15a972b78e11daa3a7e94f833399dcd28e780068 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ralph=20K=C3=BCpper?= Date: Tue, 4 Aug 2026 14:29:33 +0200 Subject: [PATCH] fix(gc): JSON.parse read its input after the source string moved 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. --- changelog.d/7373-json-parse-stale-input.md | 22 ++++++++ crates/perry-runtime/src/json/parse_api.rs | 64 +++++++++++++++++++++- 2 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 changelog.d/7373-json-parse-stale-input.md diff --git a/changelog.d/7373-json-parse-stale-input.md b/changelog.d/7373-json-parse-stale-input.md new file mode 100644 index 0000000000..1ba936f061 --- /dev/null +++ b/changelog.d/7373-json-parse-stale-input.md @@ -0,0 +1,22 @@ +### Fixed + +- **`JSON.parse` could read its input after the source string moved.** 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 that sheds parse-churn garbage between iterations — and only + *then* pushed the string's GC root and suppressed collection. An evacuating + minor at that trigger moved the string, leaving the parser reading retired + from-space for the entire parse. + + The root now precedes the trigger, and the slice is re-derived from the rooted + value afterwards. Ordering is the whole fix: pushing the root *after* the + collection roots an address the collector has already moved away from, so + re-deriving from that slot returns the same stale pointer — verified by trying + exactly that first and measuring no change. + + Invisible from output, because evacuation copies rather than zeroes and the + stale address still held the right bytes. Found with + `PERRY_GC_PROTECT_FROMSPACE=1`, which faults on the first `peek()`. + + Closes **8 of the 31** remaining quarantine catches (#7341) — the largest + single cluster. diff --git a/crates/perry-runtime/src/json/parse_api.rs b/crates/perry-runtime/src/json/parse_api.rs index 5600afc6cd..f4378c696a 100644 --- a/crates/perry-runtime/src/json/parse_api.rs +++ b/crates/perry-runtime/src/json/parse_api.rs @@ -113,11 +113,48 @@ pub unsafe fn js_json_parse_result(text_ptr: *const StringHeader) -> Result()); + std::slice::from_raw_parts(data_ptr, len) + }; let mut parser = DirectParser::new(bytes); let result = parser.parse_value(); parse_root_push(result); @@ -249,6 +286,24 @@ pub unsafe extern "C" fn js_json_parse(text_ptr: *const StringHeader) -> JSValue // in `gc_check_trigger` protects adversarial cases (previous stringify // result strings sharing blocks with interned keys) from retrigger // thrash when block-persistence keeps everything alive. + // #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)); + crate::gc::gc_check_trigger(); // Suppress GC for the duration of the parse. Parse is synchronous and @@ -257,7 +312,12 @@ pub unsafe extern "C" fn js_json_parse(text_ptr: *const StringHeader) -> JSValue // cycles walking an ever-growing live set (issue #59). crate::gc::gc_suppress(); - let text_root = parse_root_push(JSValue::string_ptr(text_ptr as *mut StringHeader)); + 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::()); + std::slice::from_raw_parts(data_ptr, len) + }; let mut parser = DirectParser::new(bytes); let result = parser.parse_value();