YAML, JSON5, XML stringify: throw instead of aborting when the output passes the string length limit - #42312
YAML, JSON5, XML stringify: throw instead of aborting when the output passes the string length limit#42312robobun wants to merge 3 commits into
Conversation
…gify output does not fit in a string The Rust `wtf::StringBuilder` records an overflow instead of crashing, and `StringBuilder__toString` throws an out-of-memory error for it. Two of the functions between those asserted on an overflowed builder and aborted the process: - `StringBuilder__ensureUnusedCapacity` read `length()`, which release-asserts `!hasOverflowed()`. Block-style `Bun.YAML.stringify` reserves capacity before every indentation run. - `StringBuilder__appendString` with a 16-bit string makes an 8-bit builder upconvert. That path reads `capacity()`, which calls `length()` when the buffer is gone. Every stringifier on this builder appends keys, names and indentation this way. Both now return early for an overflowed builder.
|
Warning Review limit reached
On-demand reviews are free for the next 9 days. After that, they cost $0.25 per reviewed file. Or wait 23 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status Reproduced on bun 1.4.3-canary (5f55496), release build. Each of these exits with code 134 ( let o = new Array(108000).fill(1);
for (let i = 0; i < 2000; i++) o = { a: o };
Bun.YAML.stringify(o, null, 10);
Bun.YAML.stringify({ a: "x".repeat(2 ** 31 - 1), "日本": 1 });
Bun.JSON5.stringify({ a: "x".repeat(2 ** 31 - 1), "日本": 1 });On a debug ASAN build with Fix: this PR, #42312. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 4:56 AM PT - Sep 11th, 2026
✅ @robobun, your commit 25bb379ad8c14095fddf94b7d09446287c261220 passed in 🧪 To try this PR locally: bunx bun-pr 42312That installs a local version of the PR into your bun-42312 --bun |
There was a problem hiding this comment.
LGTM — both earlier nits are addressed (stderr is now drained in the Promise.all for both subprocess tests, and the ASAN fixture builds its filler with Buffer.alloc(...).toString("latin1")).
What was reviewed:
- The two
hasOverflowed()guards mirror the existing check inStringBuilder__toString(same file, line 72) and protect exactly the accessors the PR description names; the otherappend*wrappers route through overloads that already check overflow, per the description's audit. - Test file:
test.eachunderdescribe.concurrent,bunEnvspread, pipes drained concurrently, stdout asserted before exit code, positive contract (threw RangeError Out of memory+ exit 0) rather than absence-of-crash — conventions match test/CLAUDE.md. - The release-only test's
"x".repeat(2**31-1)is gated behindskipIf(isDebug), so the debug-JSC.repeatslowdown doesn't apply; the 120 s per-test timeout is the "rare outlier" case CLAUDE.md allows for a 2 GiB allocation.
Extended reasoning...
Overview
The PR adds two three-line hasOverflowed() early-return guards to src/jsc/bindings/StringBuilderBinding.cpp — in StringBuilder__appendString (before the upconvert path reads capacity()) and StringBuilder__ensureUnusedCapacity (before it reads length()). Both accessors release-assert !hasOverflowed() in WTF, so an overflowed builder previously aborted the process; now the wrappers become no-ops and StringBuilder__toString (which already checks overflow at line 72 of the same file) surfaces the catchable RangeError: Out of memory. A new test file spawns child processes that force overflow via ASAN's max_allocation_size_mb cap (six concurrent cases covering both guarded calls across YAML/JSON5/XML) plus one release-only case that overflows by real length.
Security risks
None. This converts a user-reachable abort() into a catchable error, which is strictly a robustness improvement. No parsing of untrusted input, no auth/crypto/permissions surface, no new allocation or pointer handling — the guards only read a boolean and return early.
Level of scrutiny
Low-to-moderate. The native change is six lines following an established pattern already present a few lines up in the same file, with [[unlikely]] hints matching the existing guard. The PR description's audit of which StringBuilder entry points already check overflow (span/char/number/appendQuotedJSONString/reserveCapacity) versus which don't (the variadic adapter path via StringImpl*) lines up with the two functions guarded. CODEOWNERS covers only *.d.ts, so neither changed path is owned.
Other factors
Since the prior review, commits 04758eb and 25bb379 addressed both optional inline comments: stderr is now included in Promise.all for both subprocess spawns, and the ASAN fixture uses Buffer.alloc(size, "x").toString("latin1") instead of .repeat(). The "latin1" encoding is a nice touch — it keeps the builder 8-bit so the UTF-16 key still triggers upconversion. The github-actions bot thread on StringBuilderBinding.cpp:10 was replied to and resolved after the final commit; the file as it stands is clean. No outstanding CHANGES_REQUESTED reviews. The test file justifies its own existence (shared builder across four APIs, precedent cited), runs concurrently, and asserts the positive contract rather than absence of a crash.
This branch has not been deployed
1 participant
Problem
Bun.YAML.stringify,Bun.JSON5.stringifyandBun.XML.stringifyabort the process when the output does not fit in a string:panic(main thread): abort() called, exit code 134. An assertions build printsASSERTION FAILED: !hasOverflowed()inWTF::StringBuilder::length()(wtf/text/StringBuilder.h(288)).JSON.stringifythrowsRangeError: Out of memory.length()release-asserts on an overflowed builder, and two functions insrc/jsc/bindings/StringBuilderBinding.cppreach it.StringBuilder__ensureUnusedCapacity(:78) reads it. Block-style YAML calls that before each indentation run.StringBuilder__appendString(:43) reaches it when a 16-bit key or name goes into an 8-bit builder.Fix
StringBuilder__toStringalready throws the error.ReadableStream(BunStandaloneTextSink.h:35) aborts the same way after awrite()error that the caller caught. See Notes.test/js/bun/util/stringify-string-limit.test.ts(new, seven tests). Withsrc/at main the six ASAN tests abort with the assertion above. The release test fails on bun 1.4.3-canary. Also ran the YAML, TOML, JSON5 and XML suites.Background
wtf::StringBuilder(src/jsc/StringBuilder.rs) is the Rust handle for a C++WTF::StringBuilder. The YAML, JSON5, TOML and XML stringifiers write into it.OverflowPolicy::RecordOverflow: an append past 2^31 - 1 characters, or a failed allocation, makeshasOverflowed()true. The failed reallocation has already freed the buffer.capacity(), which islength()once the buffer is gone.Notes
Reproduction on bun 1.4.3-canary (5f55496). Each exits with code 134.
Where the second abort comes from.
BunString::appendToBuilderpasses aStringImpl*to the variadicStringBuilder::append.appendFromAdaptersdoes not checkhasOverflowed(). For a 16-bit string and an 8-bit builder it callsextendBufferForAppendingWithUpconvert, which computesexpandedCapacity(capacity(), requiredLength).capacity()ism_buffer ? m_buffer->length() : length(), and a failedtryReallocatehas already releasedm_buffer. Theappend(std::span<...>)overloads,append(char16_t),append(Latin1Character), the number adapters (8-bit, so they take the checkedextendBufferForAppendingpath),appendQuotedJSONStringandreserveCapacityall check first.Bun.TOML.stringifywrites non-ASCII keys and values one character at a time, so it did not abort.The tests.
max_allocation_size_mb=4withMalloc=1makes the builder's next doubling fail, which calls the samedidOverflow()and leaves the same state.text-encoder-stream.test.ts,buffer-oom.test.tsandzstd.test.tsuse the same ASAN options. bun boots with a cap as low as 1 MiB. Each child takes about 1 s on a debug build, and the six run concurrently.ensureUnusedCapacitycheck, those five still abort.threw RangeError Out of memoryon the fixed debug build when run by hand (220 s).streams-string-limit.test.tsandsource-too-large.test.ts. The file takes about 4 s on a debug build.XML.stringify > deep values are a catchable error(7.7 s) andstack overflow protection in the write passinyaml.test.ts(4.4 to 5.8 s). The XML one does the same withsrc/at main. Both build a 1,000,000-deep value and neither reaches an overflowed builder. Everything else in the four suites passes.Self-review, by concern.
ensureUnusedCapacity. The review found that a 16-bit key or indent after the overflow still aborted, in YAML with and without a space argument, in JSON5 and in XML. That is theappendStringcheck and the five rows (2 concerns).has_overflowed()accessor. It had no test of its own and it touched the same lines as Implement the replacer argument of YAML, TOML and JSON5 stringify #39925. It is gone. The diff is now the two checks and nothing on the Rust side (2 concerns).ensure_unused_capacitycalls inYAMLObject.rsstay as they are.WTF::StringBuilder::reserveCapacityreallocates to the exact size, so they probably do not help, and JSON5 and XML have the samenewline()without them. Removing them is a performance change with no measurement behind it, and Implement the replacer argument of YAML, TOML and JSON5 stringify #39925 rewrites those functions and keeps the calls, so it is not part of this crash fix (rejected). The first version also refused a reservation aboveString::MaxLength. That is gone too: such a request makes the builder record an overflow, and the result is the same catchable error (2 concerns).The stream text sink (not fixed here).
BunTextAccumulator::ropeis the only otherRecordOverflowbuilder insrc/.writeToTextSink(JSDirectStreamController.cpp:405) throwsRangeError: Out of memorywhen an append overflows it, but leaves it overflowed. If the caller catches that and writes a typed array, line 431 callsrope.toString(), which asserts inreifyString().BunStreamConsumers.cpp:679,:709and:744readrope.length()the same way. UnderMalloc=1andmax_allocation_size_mb=4on a debug build:The fix has to decide what a write after a failed write does (throw again, or error the stream), in two write arms and two end arms. It belongs with the stream code and its tests.
[human-review] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file