jsc: throw RangeError from JSON.parse when a string value cannot be allocated - #35194
jsc: throw RangeError from JSON.parse when a string value cannot be allocated#35194robobun wants to merge 6 commits into
Conversation
…llocated JSON.parse copies every string value longer than 16 chars into a fresh WTF::StringImpl via JSONAtomStringCache::makeJSString. The copy used the crash-or-succeed StringImplMalloc::malloc path, so a near-OOM process died inside fastCompactMalloc instead of throwing. Seen in Sentry as BUN-2Z94 (Windows), reproducible on Linux by parsing a ~200 MB quoted string under RLIMIT_AS with the rest of the address space filled. The fix lives in oven-sh/WebKit#317: the >16 char branch now goes through StringImpl::tryCreateUninitialized and a null result surfaces as throwOutOfMemoryError in LiteralParser::parsePrimitiveValue. This bumps WEBKIT_VERSION to that change and adds a Linux-only test that runs the fixture under ulimit -v (skipped under ASAN because the shadow reservation cannot coexist with RLIMIT_AS).
|
Reproduced on Linux under The code change is in oven-sh/WebKit#317 (now CI is green on build #78314: the new Merge gate: |
|
Updated 10:57 PM PT - Jul 22nd, 2026
✅ @robobun, your commit 4b69972d32be2a4b603a304f393cd848be0dd1ae passed in 🧪 To try this PR locally: bunx bun-pr 35194That installs a local version of the PR into your bun-35194 --bun |
WalkthroughThe change selects a new WebKit Autobuild preview tag and adds Linux-only memory-constrained coverage verifying that ChangesWebKit preview dependency
JSON.parse out-of-memory handling
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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: 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 `@test/js/bun/util/json-parse-oom-fixture.js`:
- Around line 14-23: The JSON OOM fixture currently exercises only a root
string; parameterize it to construct selectable root-string, array-element, and
object-value wrappers around the large payload while preserving setup failure
handling. In test/js/bun/util/json-parse-oom-fixture.js lines 14-23, update the
fixture input construction to accept the wrapper selection; in
test/js/bun/util/json-parse-oom.test.ts lines 17-22, iterate over every wrapper
variant and payload size, applying the existing no-crash and RangeError
assertions to each combination.
In `@test/js/bun/util/json-parse-oom.test.ts`:
- Around line 51-58: Update the test branch handling the selected fixture
outcome so the CAUGHT path asserts exitCode is 0 and the PARSED path asserts
exitCode is 1. Remove the shared expect([0, 1]).toContain(exitCode) assertion
while preserving the existing stdout validations and sawCaught assignment.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 927fd5e4-f726-4440-9a1e-3cdecece0428
📒 Files selected for processing (3)
scripts/build/deps/webkit.tstest/js/bun/util/json-parse-oom-fixture.jstest/js/bun/util/json-parse-oom.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // typed-array constructor ClassInfo kept address-unique under LTO, and the | ||
| // Windows ICU data table filtered + per-item zstd compressed. | ||
| export const WEBKIT_VERSION = "c9296e353e365ecf0de82f273bb0a88a3df465be"; | ||
| export const WEBKIT_VERSION = "autobuild-preview-pr-317-d9b06a28"; |
There was a problem hiding this comment.
🔴 WEBKIT_VERSION is pinned to autobuild-preview-pr-317-d9b06a28, a PR-preview release tag rather than a merged oven-sh/WebKit main sha (contradicting the comment directly above). Preview-PR release artifacts can be garbage-collected once WebKit#317 merges/closes, at which point prebuiltUrl() will 404 and fresh clones of Bun stop building. The PR description already notes this needs updating — flagging so it isn't merged before the pin moves to the merged main sha.
Extended reasoning...
What the bug is
WEBKIT_VERSION in scripts/build/deps/webkit.ts:11 is set to "autobuild-preview-pr-317-d9b06a28", a CI preview tag produced for oven-sh/WebKit#317, rather than a commit sha on oven-sh/WebKit main. The comment block directly above the constant documents the invariant this violates: // oven-sh/WebKit main: macOS + Windows artifacts cross-compiled on Linux, .... The PR description acknowledges this is temporary ("Depends on oven-sh/WebKit#317. WEBKIT_VERSION points at its preview tag; once that PR is merged the pin should move to the merged main sha"), so the intent is clear — this comment is a merge-gate reminder, not a disagreement.
Code path that triggers it
prebuiltUrl(cfg) constructs the download URL directly from cfg.webkitVersion:
const tag = version.startsWith("autobuild-") ? version : `autobuild-${version}`;
return `https://github.com/oven-sh/WebKit/releases/download/${tag}/${name}.tar.gz`;and prebuiltDestDir(cfg) keys the extraction cache on the same value. The build system does branch on the autobuild- prefix, so the preview tag works today — the tarball resolves and extracts correctly. The problem is durability, not correctness-right-now.
Why nothing prevents it
Nothing in scripts/build/deps/webkit.ts validates that WEBKIT_VERSION points at a stable release. The autobuild- handling in prebuiltUrl/prebuiltDestDir was added precisely so preview tags can be tested locally via --webkit-version=<tag>, but committing one as the default pin means every consumer of cfg.webkit === "prebuilt" (the default mode — local mode requires a manual 10+ min clone) depends on that release existing.
Impact
Preview-PR release artifacts on oven-sh/WebKit are ephemeral: once PR #317 merges or closes, its autobuild-preview-pr-317-* release can be pruned by the release workflow. After that point:
- A developer runs
bun bdon a fresh clone (or afterrm -rf build/clears the cache dir). resolveDepfor WebKit hits theprebuiltbranch and callsprebuiltUrl(cfg)→https://github.com/oven-sh/WebKit/releases/download/autobuild-preview-pr-317-d9b06a28/bun-webkit-linux-amd64-debug.tar.gz.- GitHub returns 404; the build fails before compiling a single source file.
Anyone with the tarball already cached (identity autobuild-preview-pr-317-d9b06a28-debug) is unaffected, which is exactly why this class of pin tends to slip through — it works on the author's and reviewers' machines and only breaks for new contributors / fresh CI runners later.
Step-by-step proof
WEBKIT_VERSION = "autobuild-preview-pr-317-d9b06a28"(line 11).Config.webkitVersiondefaults toWEBKIT_VERSIONunless overridden by--webkit-version.- Default
cfg.webkitis"prebuilt", sowebkit.source(cfg)returns{ kind: "prebuilt", url: prebuiltUrl(cfg), identity: "autobuild-preview-pr-317-d9b06a28-debug", destDir: ... }. prebuiltUrlseesversion.startsWith("autobuild-")→ uses the tag verbatim → URL points at theautobuild-preview-pr-317-d9b06a28GitHub release.- That release exists only while the preview workflow keeps it. Once LiteralParser: throw RangeError on OOM when copying a JSON string value WebKit#317 merges and preview releases are cleaned up, step 4's URL 404s and
bun bdfails at dependency fetch.
How to fix
Before merging this PR, land oven-sh/WebKit#317 and update line 11 to the resulting 40-hex main sha (matching the format of the previous pin c9296e353e365ecf0de82f273bb0a88a3df465be and the // oven-sh/WebKit main comment above it). No other change needed — prebuiltUrl/prebuiltDestDir already handle both forms.
There was a problem hiding this comment.
Agreed, and the PR body already calls this out as the merge-gate step. Leaving this thread open as the reminder: once oven-sh/WebKit#317 lands on main this line moves to the merged sha before this PR is merged.
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 `@test/js/bun/util/json-parse-oom-fixture.js`:
- Around line 16-19: Validate the fixture arguments before constructing or
allocating the JSON payload: require N to be a positive safe integer and reject
invalid values before Buffer.alloc, and require shape to be exactly "root",
"array", or "object" rather than silently treating unknown or empty values as
root. Preserve the root default only when the shape argument is omitted, using
presence-based defaulting around the existing shape/prefix/suffix logic.
In `@test/js/bun/util/json-parse-oom.test.ts`:
- Around line 24-29: Add a reviver mode to the OOM test case matrix and
propagate it through each fixture invocation so one case exercises JSON.parse
with a reviver. Assert that the reviver case preserves the same
no-signal/RangeError contract as the existing root, array, and object cases.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 266b0451-4b5f-4a97-9b63-fd4c699e76ba
📒 Files selected for processing (2)
test/js/bun/util/json-parse-oom-fixture.jstest/js/bun/util/json-parse-oom.test.ts
…t timeout allocUnsafe for the filler reserves address space without committing pages, so five children can run in parallel with low RSS. Skip on debug builds where the ~200 MB input construction alone takes seconds; the release Linux lanes are what this test targets.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/bun/util/json-parse-oom.test.ts (1)
58-86: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire every selected shape to reach
INPUT-OK.A setup failure in
reviver,array, orobjectis silently skipped; a root case can make both global flags pass. Track reached shapes and assert all unique shapes reached parsing, so the new reviver coverage is actually exercised.Proposed fix
- let sawInputOK = false; + const reachedShapes = new Set<string>(); for (const { shape, size, stdout, stderr, exitCode, signal } of results) { if (!stdout.includes("INPUT-OK")) continue; - sawInputOK = true; + reachedShapes.add(shape); // ... } - expect(sawInputOK).toBe(true); + expect(reachedShapes).toEqual(new Set(cases.map(([shape]) => shape)));As per coding guidelines, tests must assert setup preconditions and cover the complete variant matrix. Based on PR objectives, reviver and each JSON container context are required coverage.
🤖 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 `@test/js/bun/util/json-parse-oom.test.ts` around lines 58 - 86, Update the result sweep around sawInputOK and sawCaught to track each selected shape that reaches INPUT-OK, rather than allowing skipped reviver, array, or object cases to pass via a root case. After processing results, assert that every unique selected shape was reached and preserve the existing JSON.parse outcome and out-of-memory assertions.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/js/bun/util/json-parse-oom.test.ts`:
- Around line 58-86: Update the result sweep around sawInputOK and sawCaught to
track each selected shape that reaches INPUT-OK, rather than allowing skipped
reviver, array, or object cases to pass via a root case. After processing
results, assert that every unique selected shape was reached and preserve the
existing JSON.parse outcome and out-of-memory assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2149a32b-5dad-44f4-acde-cecd52a85d48
📒 Files selected for processing (2)
test/js/bun/util/json-parse-oom-fixture.jstest/js/bun/util/json-parse-oom.test.ts
1 participant
What does this PR do?
JSON.parsecopies every string value longer than 16 chars into a freshWTF::StringImplviaJSONAtomStringCache::makeJSString. That copy usedString(characters), which lands inStringImplMalloc::malloc, the crash-or-succeed allocator. When the process cannot satisfy that allocation,JSON.parsedies insidefastCompactMallocinstead of throwing. Seen in Sentry as BUN-2Z94 (Windows null deref infastCompactMalloc).The fix itself is in oven-sh/WebKit#317: the >16 char branch now goes through
StringImpl::tryCreateUninitializedand a null result surfaces asthrowOutOfMemoryError(m_globalObject, scope)inLiteralParser::parsePrimitiveValue. All ofparsePrimitiveValue's callers already treat an empty return with a pending exception correctly (they assert(!!scope.exception() || !m_parseErrorMessage.isNull()) == !valueand the top-levelJSON.parsecallers checkRETURN_IF_EXCEPTIONbefore throwingSyntaxError).This PR bumps
WEBKIT_VERSIONto that change and adds a test.Why
Crashing on catchable OOM is worse than throwing.
String.prototype.repeat,Buffer#toString, and every other WTF-string-backed allocation already throwRangeError: Out of memory;JSON.parsestring values were the odd one out because the copy happened inside JSC's literal parser on the non-try path.How did you verify your code works?
Reproduced on Linux by parsing a ~200 MB quoted string under
ulimit -vwith the rest of the address space filled so the value copy cannot fit:Stable across 5 runs each. Also verified the fix covers string values as top-level literals, array elements, object property values, and under a reviver (all go through
parsePrimitiveValue).The test spawns the fixture under
ulimit -vand sweeps four string sizes; for every run that reachesINPUT-OKit asserts the child either parsed or threwRangeErrorand was never signal-killed.RLIMIT_AScannot coexist with AddressSanitizer's ~16 TB shadow reservation, so the test is gatedskipIf(!isLinux || isASAN).Fail-before proof
The fix is a WebKit change pulled in via
WEBKIT_VERSION; with the fix reverted toc9296e35the test fails on the first 200 MB run (signalCode: "SIGILL"). The harness fail-before step stashessrc/only, so it cannot mechanically revert the prebuilt-WebKit pin; the before/after evidence above stands in for that check.Depends on oven-sh/WebKit#317.
WEBKIT_VERSIONpoints at its preview tag; once that PR is merged the pin should move to the merged main sha.no test proof · iteration 1 · Platform-specific test-only change; deferring to CI.