fix(json): resolve GC_FLAG_FORWARDED array stubs in pretty/replacer stringify - #7713
Conversation
…tringify JSON.stringify(v, null, 2) on an array grown past its initial inline capacity returned nondeterministic garbage (occasionally crashing) because several call sites in json/replacer.rs cast an array pointer straight to *ArrayHeader without following the js_array_grow (#233) forwarding chain that clean_arr_ptr resolves. The plain (non-pretty) stringify path already did this correctly; the pretty-print/replacer paths did not. Fixes #7269.
📝 WalkthroughWalkthroughPretty JSON array paths now resolve forwarding pointers created by array growth. The change covers replacers, fallback serialization, PropertyList extraction, and array detection. A regression test validates stale-pointer serialization. Project version metadata changes to ChangesJSON array pointer cleanup
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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 |
![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/replacer.rs`:
- Around line 894-899: After clean_arr_ptr resolves successfully, stop using the
original ptr and consistently use resolved for the object-pointer check,
stringify_array_pretty call, and all fallback length/capacity accesses. Ensure
every dereference after clean_arr_ptr is based on the resolved pointer so
materialization cannot leave the code inspecting an invalid raw pointer.
🪄 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: 2f03bccf-8628-4efa-a104-7b7419c6a19a
📒 Files selected for processing (2)
changelog.d/7713-json-replacer-clean-ptr.mdcrates/perry-runtime/src/json/replacer.rs
| let resolved = crate::array::clean_arr_ptr(ptr as *const crate::ArrayHeader); | ||
| if !resolved.is_null() { | ||
| let len = (*resolved).length; | ||
| let cap = (*resolved).capacity; | ||
| if len <= cap && cap > 0 && cap < 10000 && !is_object_pointer(ptr) { | ||
| stringify_array_pretty(ptr, buf, indent, depth); | ||
| stringify_array_pretty(resolved as *const u8, buf, indent, depth); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use the resolved pointer after clean_arr_ptr.
clean_arr_ptr can materialize a lazy array and allocate. It returns the current array pointer. Lines 898 and 903-907 still inspect the original ptr. A collection during materialization can invalidate that raw pointer and cause an invalid dereference.
Use resolved as *const u8 for the object probe, serialization call, and fallback accesses when resolution succeeds.
Proposed fix
let resolved = crate::array::clean_arr_ptr(ptr as *const crate::ArrayHeader);
+let current_ptr = if resolved.is_null() {
+ ptr
+} else {
+ resolved as *const u8
+};
if !resolved.is_null() {
let len = (*resolved).length;
let cap = (*resolved).capacity;
- if len <= cap && cap > 0 && cap < 10000 && !is_object_pointer(ptr) {
- stringify_array_pretty(resolved as *const u8, buf, indent, depth);
+ if len <= cap && cap > 0 && cap < 10000 && !is_object_pointer(current_ptr) {
+ stringify_array_pretty(current_ptr, buf, indent, depth);
return;
}
}
-if is_object_pointer(ptr) {
- stringify_object_pretty(ptr, buf, indent, depth);
+if is_object_pointer(current_ptr) {
+ stringify_object_pretty(current_ptr, buf, indent, depth);
} else {
- let str_ptr = ptr as *const StringHeader;
+ let str_ptr = current_ptr as *const StringHeader;Based on learnings: raw Rust pointer locals are neither GC roots nor reliable pins after an allocating operation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let resolved = crate::array::clean_arr_ptr(ptr as *const crate::ArrayHeader); | |
| if !resolved.is_null() { | |
| let len = (*resolved).length; | |
| let cap = (*resolved).capacity; | |
| if len <= cap && cap > 0 && cap < 10000 && !is_object_pointer(ptr) { | |
| stringify_array_pretty(ptr, buf, indent, depth); | |
| stringify_array_pretty(resolved as *const u8, buf, indent, depth); | |
| let resolved = crate::array::clean_arr_ptr(ptr as *const crate::ArrayHeader); | |
| let current_ptr = if resolved.is_null() { | |
| ptr | |
| } else { | |
| resolved as *const u8 | |
| }; | |
| if !resolved.is_null() { | |
| let len = (*resolved).length; | |
| let cap = (*resolved).capacity; | |
| if len <= cap && cap > 0 && cap < 10000 && !is_object_pointer(current_ptr) { | |
| stringify_array_pretty(current_ptr, buf, indent, depth); |
🤖 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/replacer.rs` around lines 894 - 899, After
clean_arr_ptr resolves successfully, stop using the original ptr and
consistently use resolved for the object-pointer check, stringify_array_pretty
call, and all fallback length/capacity accesses. Ensure every dereference after
clean_arr_ptr is based on the resolved pointer so materialization cannot leave
the code inspecting an invalid raw pointer.
Source: Learnings
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 `@Cargo.toml`:
- Line 318: Revert the contributor-added release metadata changes at Cargo.toml
lines 318-318 and CLAUDE.md lines 11-11: restore the workspace version and
documented current version to their pre-change values, leaving maintainer-owned
version updates for the PR merge/release workflow.
🪄 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: 753701df-f7b7-4c28-8c47-b38f2e1836f4
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
CLAUDE.mdCargo.toml
|
|
||
| [workspace.package] | ||
| version = "0.5.1413" | ||
| version = "0.5.1414" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for file in CONTRIBUTING.md CLAUDE.md Cargo.toml; do
if [ -f "$file" ]; then
rg -n -C 3 'workspace\.package|Current Version|maintainer|release|version' "$file"
fi
doneRepository: PerryTS/perry
Length of output: 22003
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- README version/bump related sections ---\n'
sed -n '1,95p' README.md
printf '\n--- CLAUDE first 15 lines ---\n'
sed -n '1,15p' CLAUDE.md
printf '\n--- Cargo workspace package section ---\n'
sed -n '316,319p' Cargo.toml
printf '\n--- Git status/diff summary ---\n'
git status --short
git diff --stat
git diff -- Cargo.toml CLAUDE.md README.mdRepository: PerryTS/perry
Length of output: 242
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- README version/bump related sections ---'
sed -n '1,95p' README.md
printf '%s\n' ''
printf '%s\n' '--- CLAUDE first 15 lines ---'
sed -n '1,15p' CLAUDE.md
printf '%s\n' ''
printf '%s\n' '--- Cargo workspace package section ---'
sed -n '316,319p' Cargo.toml
printf '%s\n' ''
printf '%s\n' '--- Git status/diff summary ---'
git status --short
git diff --stat
git diff -- Cargo.toml CLAUDE.md README.md || trueRepository: PerryTS/perry
Length of output: 7981
Align the contributed release metadata with Perry’s PR workflow.
README.md says maintainers bump [workspace.package].version in Cargo.toml and **Current Version:** in CLAUDE.md at merge/release. Revert these contributor-added metadata edits if this is an external contributor PR.
Cargo.toml#L318-L318: Revert the workspace version if maintainers own version updates.- and
CLAUDE.md#L11-L11: Revert the documented version if maintainers own release metadata.
📍 Affects 2 files
Cargo.toml#L318-L318(this comment)CLAUDE.md#L11-L11
🤖 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 `@Cargo.toml` at line 318, Revert the contributor-added release metadata
changes at Cargo.toml lines 318-318 and CLAUDE.md lines 11-11: restore the
workspace version and documented current version to their pre-change values,
leaving maintainer-owned version updates for the PR merge/release workflow.
Sources: Coding guidelines, Learnings
Merging as v0.5.1414Good result, and the interesting parts are the two places it departs from the issue. The issue's citation list was partly wrong, and the PR says so rather than fixing four things to match it. Of the four sites cited (at drifted line numbers):
And tracing the call graph turned up two more of the same bug class on the replacer-array argument ( The test is deterministic, which is better than the issue's own repro. "Nondeterministic garbage" invites a test that passes by luck. Instead it grows a real array past capacity and relies on It then asserts the stale header's raw The sabotage is the strongest part. Reverting only the
|
1 participant
Summary
JSON.stringify(v, null, 2)on an array grown past its initial inline capacity (MIN_ARRAY_CAPACITY, 16) returned nondeterministic garbage — a different, wrong-length string on every run of the same binary, occasionally crashing.js_array_grow(Array.push from inside an async function silently caps at 16 elements when the array is a function parameter #233) reallocates and leaves aGC_FLAG_FORWARDEDstub at the OLD address. The stub's first 8 bytes are exactlyArrayHeader.length+.capacity, now holding the raw forwarding pointer to the new array. Several call sites incrates/perry-runtime/src/json/replacer.rscast an array pointer straight to*ArrayHeaderwithout following that chain viacrate::array::clean_arr_ptr— the plain (non-pretty) stringify path (json/stringify.rs::stringify_array_depth) already did this correctly.crates/perry-runtime/src/json/replacer.rs):stringify_value_pretty'sTYPE_UNKNOWNstructural-fallback probe — the site actually hit by the issue's exact repro (JSON.stringify(v, null, 2), no replacer). This is the necessary fix; reverting only this one is enough to make the new test fail.stringify_array_pretty— hardened as the shared choke point for both its callers, so it no longer depends on a caller having resolved first.stringify_array_with_replacer_pretty— was safe today only because its one current caller happens to resolve first; fixed so it doesn't rely on that invariant.extract_string_arrayandis_array_value— the replacer array argument (JSON.stringify(v, ['a','b'])) has the identical hazard on its own pointer; fixed while in the area.stringify_array_with_array_replacer, already resolved viaclean_arr_ptrbefore this issue was filed and needed no change — noted in the changelog fragment.Test plan
pretty_stringify_resolves_array_grown_past_inline_capacity(crates/perry-runtime/src/json/replacer.rs): grows a real array past its allocated capacity (no GC cycle needed —js_array_growinstalls the forwarding stub unconditionally on every reallocating grow), asserts the stale header's raw(length, capacity)bytes reconstruct the grown array's exact address (sabotage precondition), then pretty-stringifies the stale pre-grow pointer and asserts the output matches the array's real, current contents.stringify_value_prettyresolve alone (keeping thestringify_array_prettydefense-in-depth resolve) still made the test process abort (SIGABRT, "thread panicked while processing panic") — the misreadlength(up to ~4 billion) drove the pretty-printer into a deep, self-feeding recursion over adjacent heap bytes reinterpreted as more NaN-boxed values, overflowing the stack. Restored the fix afterward and confirmed a clean pass.cargo test -p perry-runtime --lib --no-fail-fast: 1957 passed, 0 failed, 4 ignored.cargo fmt --all -- --check: clean.Summary by CodeRabbit
Bug Fixes
nullor an empty key list as appropriate.Tests