TextDecoder: throw ERR_STRING_TOO_LONG instead of returning an empty string - #37216
TextDecoder: throw ERR_STRING_TOO_LONG instead of returning an empty string#37216robobun wants to merge 3 commits into
Conversation
WalkthroughThe PR adds JavaScript string-length validation, throws ChangesString length safety
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:20 PM PT - Aug 8th, 2026
❌ @robobun, your commit bbf4670 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 37216That installs a local version of the PR into your bun-37216 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
Verified the #26323 connection on Linux x64 with a 2147483649-byte ASCII file: current main dumps core in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline nit, I also checked: the remaining 2^32-1 characters assertions in blob-oom.test.ts are for the JSON-parse message (bindings.cpp:2534), not the create-string message, so they're unaffected. ZigString__toValueGC newly throwing is consistent with to_js's other branch (to_external_value), which already threw + returned .ZERO, so callers already handle that shape.
Extended reasoning...
Checked that the error-message change didn't miss any test assertions (grep for 2^32-1 characters in test/ — remaining hits are the untouched JSON-parse path), and that making ZigString__toValueGC throwing doesn't introduce a new caller contract (the sibling to_external_value arm of ZigStringJsc::to_js already had throw-and-return-ZERO semantics). Not approving because the change touches core string→JS conversion and String::max_length() semantics used across many paths; the inline nit plus a human look at CI is appropriate.
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
@robobun close or rebase |
…string Decoding 2^31 .. 2^32-1 bytes of ASCII with TextDecoder.decode() returned the empty string with no error: Zig::toStringCopy maps a failed string creation (length over WTF::StringImpl::MaxLength, or allocation failure) to a null WTF::String, and jsString() turns that into the empty string. Zig::toJSStringGC (and with it ZigString__toValueGC, i.e. ZigString.toJS) now throws ERR_STRING_TOO_LONG when the input is over the length limit and an out-of-memory error when allocation fails, instead of silently returning "". The non-UTF-8 branches of the single-argument Zig::toStringCopy now check the synthetic allocation limit like the other helpers, and JSC__JSValue__fromEntries checks for the exception before putDirect. The sibling guards on the external-string paths (2 GiB to 4 GiB aborts) are fixed separately in #37215.
dfdcd88 to
e926765
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Rebased and narrowed. This PR and #37215 were opened minutes apart for sibling bugs with overlapping fixes; this one now contains only the parts #37215 does not cover: the silent-empty-string path ( |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@src/jsc/bindings/helpers.h`:
- Around line 261-271: Update the toStringCopy failure handling to distinguish
decoded UTF-16 length from the input UTF-8 byte length before selecting the
exception. In the wtfString null branch, use the conversion status or retained
decoded-length result so inputs whose decoded output is within the limit report
out-of-memory, while genuinely over-limit strings still use
Bun::ERR::STRING_TOO_LONG.
In `@test/js/web/encoding/text-decoder.test.js`:
- Around line 965-973: Remove the explicit 30-second timeout from the test
containing the 2**31-byte ASCII input. Measure its runtime in debug and ASAN
builds; if it exceeds the default test budget, reduce or conditionally gate the
physical-input case while preserving the synthetic regression coverage.
- Around line 966-972: Update both subprocess tests at
test/js/web/encoding/text-decoder.test.js:966-972 and :978-984 to configure
Bun.spawn with stderr: "pipe" and concurrently await proc.stderr.text()
alongside proc.stdout.text() and proc.exited. Keep assertions based on stdout
and exitCode, without requiring stderr to be empty.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 31586b07-cd8b-4293-8da2-52499210359c
📒 Files selected for processing (3)
src/jsc/bindings/bindings.cppsrc/jsc/bindings/helpers.htest/js/web/encoding/text-decoder.test.js
…eout For UTF-8-tagged input the byte length can exceed the limit while the decoded UTF-16 length is within it; recompute the decoded length in the failure path so a genuine allocation failure reports out-of-memory rather than ERR_STRING_TOO_LONG. Pipe and drain stderr in the new subprocess tests and rely on the default test timeout (the 2 GiB case runs in about a second).
The Uint8Array reservation itself can fail there, which would report an allocation error instead of exercising the decode path. Same gate as blob-oom.test.ts and fs-oom.test.ts; the synthetic-limit variant keeps the coverage on small machines.
There was a problem hiding this comment.
All prior review feedback (mine and CodeRabbit's) is addressed in b70c1fc and bbf4670, and this pass found no new issues. Deferring to a human because the fix changes the contract of Zig::toJSStringGC / ZigString__toValueGC — a core string-conversion helper reached from ZigString::to_js across the Rust codebase — from never-throws to throws-and-returns-nullptr.
What was reviewed:
toJSStringGC's three call sites (ZigString__toValueGC,JSC__JSValue__fromEntries,Bun::toJS's ZigString arm) — each now sees the exception before using the result.- The forward declaration of
Bun::ERR::STRING_TOO_LONGmatches ErrorCode.h:112 exactly. - The 2 GiB test now gates on
it.skipIf(os.totalmem() < 10 * 1024 ** 3)and both subprocess tests pipe/drain stderr.
Extended reasoning...
Overview
The PR fixes TextDecoder.decode() silently returning "" when the decoded string would be 2^31..2^32-1 characters. It touches src/jsc/bindings/helpers.h (adds a length check to toStringCopy, rewrites toJSStringGC to throw ERR_STRING_TOO_LONG/OOM on a null WTF::String instead of passing it to jsString(), and forward-declares Bun::ERR::STRING_TOO_LONG to avoid an include cycle), src/jsc/bindings/bindings.cpp (ZigString__toValueGC delegates to toJSStringGC; JSC__JSValue__fromEntries checks the exception before putDirect), and adds two subprocess tests to text-decoder.test.js.
Security risks
None identified. The change tightens behavior (fail loud instead of silent empty string) on a boundary condition. No new user-controlled data reaches parsing or allocation paths that weren't already reachable.
Level of scrutiny
Medium-high. helpers.h is included transitively by essentially every C++ binding file, and ZigString__toValueGC backs ZigString::to_js which is called throughout the Rust runtime (src/jsc/lib.rs:1633, src/runtime/node/node_os.rs:103, etc.). The contract change from "always returns a valid JSValue (possibly empty string)" to "may throw and return encoded-nullptr" is correct — it matches what Bun::toJS's Dead arm already does at BunString.cpp:176 — but it means any Rust caller that previously relied on an infallible return now sees JSValue(0) with a pending exception on oversize input. In practice this only fires above 2^31 characters (or the synthetic limit), so existing callers passing normal-sized strings are unaffected, and the old behavior (silent "") was the bug being fixed. Still, a maintainer should confirm they're comfortable with this contract change landing without auditing every to_js call site.
Other factors
This is my third look at the PR. My earlier findings (missing skipIf memory gate on the 2 GiB test; stderr not piped) and CodeRabbit's (UTF-8 byte-length vs. decoded-length classification; per-test timeout; stderr draining) are all resolved in the current diff. The comment-cop flags on long comments were also addressed. The forward declaration in helpers.h:32 matches the real declaration in ErrorCode.h:112. The gate evidence in the PR description shows the test file failing on main and passing with the fix under both debug+ASAN and release. The diff no longer overlaps with #37215. Nothing new surfaced this run; the deferral is purely because of the breadth of the helper being changed, not any specific concern.
There was a problem hiding this comment.
All prior feedback (mine, CodeRabbit's, and comment-cop's) has been addressed in b70c1fc and bbf4670 — the toJSStringGC sibling now carries the fix, the 2 GiB test is gated on os.totalmem(), stderr is piped and drained, the per-test timeout is gone, and the UTF-8 too-long/OOM classification uses the decoded length. No new issues found this pass. Deferring to a maintainer only because this changes the contract of Zig::toJSStringGC (now throws and returns nullptr) in a header transitively included across the JSC bindings, and adds a forward declaration of Bun::ERR::STRING_TOO_LONG into helpers.h to avoid a circular include — both look correct, but worth a maintainer glance.
Extended reasoning...
Overview
The PR fixes TextDecoder.decode() silently returning "" for outputs whose length falls in [2^31, 2^32-1]. It touches three files: src/jsc/bindings/helpers.h (~34 lines: toJSStringGC now throws ERR_STRING_TOO_LONG/OOM on a null WTF::String and returns nullptr; single-arg toStringCopy gains the synthetic-limit check its siblings already have; a forward decl of Bun::ERR::STRING_TOO_LONG avoids a circular include), src/jsc/bindings/bindings.cpp (~10 lines: ZigString__toValueGC delegates to toJSStringGC; JSC__JSValue__fromEntries materializes the value and checks the exception before putDirect), and two new subprocess tests in text-decoder.test.js.
Security risks
None. This tightens a silent-failure path into a thrown error; no auth, crypto, or untrusted-input parsing is touched.
Level of scrutiny
Medium-high. helpers.h is included transitively by essentially every JSC binding TU, and toJSStringGC is called from Bun::toJS's ZigString arm (BunString.cpp:194) and ZigString__toValueGC, which back bun_core::String::to_js and ZigString::to_js on the Rust side. Changing it from "returns empty JSString" to "throws and returns nullptr" is the correct JSC convention (matching the existing Dead-tag arm at BunString.cpp:176-178), and the PR already updated the one caller (fromEntries) that would have putDirect'd an empty JSValue — but a maintainer should confirm they're comfortable with the forward-declaration-in-helpers.h approach and the broader contract change.
Other factors
- All eight prior review threads (my two, CodeRabbit's three, comment-cop's three) are resolved and verified against the current diff.
- The forward declaration at helpers.h:32 matches ErrorCode.h:112 exactly.
- Tests are properly gated (
it.skipIf(os.totalmem() < 10 * 1024 ** 3)), pipe/drain stderr concurrently, usebunEnv, and have a cheap synthetic-limit variant so coverage survives on small runners. - PR evidence shows fail-without-fix / pass-with-fix on both debug+ASAN and release.
- Jarred engaged ("close or rebase") and the author rebased/narrowed to remove overlap with #37215; no maintainer approval yet.
|
CI on bbf4670 finished 195/196 green. The one failed job is "step failed outside runner" on the darwin 26 aarch64 lane, a pre-existing infra failure (the machine is currently unreachable); it also occurs on main and is unrelated to this change. The new TextDecoder tests passed on every lane that ran them. All review feedback is addressed and resolved; this is ready for a maintainer. |
2 participants
Repro
On 1.4.0-canary this prints
2147483647 -> 2147483647, then-> 0for every larger size, with exit code 0: any decode whose output is 2^31 .. 2^32-1 characters silently returns the empty string. Node throwsERR_STRING_TOO_LONGfor sizes over its limit.Cause
The all-ASCII decode path lands in
ZigString__toValueGC(ZigString.toJS), whereZig::toStringCopyreturns a nullWTF::Stringwhen the length exceedsWTF::StringImpl::MaxLength(2^31-1) or allocation fails, andjsString()turns that null string into the empty string. No error was raised anywhere.Fix
Zig::toJSStringGC(and with itZigString__toValueGC, which now delegates to it) throwsERR_STRING_TOO_LONGwhen the input is over the length limit, and an out-of-memory error when the length was fine but allocation failed, instead of returning"". Its other caller,Bun::toJS'sZigStringarm, already returns nullptr-with-pending-exception for dead strings, so the contract is unchanged.Zig::toStringCopynow check the synthetic allocation limit, matching the other helpers inhelpers.h.JSC__JSValue__fromEntriesmaterializes the value and checks for the exception beforeputDirect(a nullptrJSString*would have put an emptyJSValue).decode(new Uint8Array(2 ** 31 - 1))still returns a 2^31-1-length string.This PR is scoped to the silent-empty-string path. The sibling bug on the external-string paths (lengths in 2^31 .. 2^32-1 aborting the process, e.g.
fs.readFileSyncof a >2 GiB file, #26323) is fixed separately in #37215, which capsString::max_length()atWTF::StringImpl::MaxLength; the two PRs no longer share any hunks and merge independently in either order.Verification
New tests in
test/js/web/encoding/text-decoder.test.js: a 2^31-byte decode must throwERR_STRING_TOO_LONG(the zeroed input is never written, so the 2 GiB stay untouched zero pages and the test runs in under a second), plus a cheap variant viaBUN_FEATURE_FLAG_SYNTHETIC_MEMORY_LIMITthat covers the same path with a 160 MB input. Both fail on current main (returned:0/returned:167772160) and pass with this change. The fulltext-decoder.test.js(127 tests) andblob-oom.test.tspass with the debug build.[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 3 passed · 0 rejected · iteration 2
evidence per changed file