Check base64 output length before encoding and cover btoa, StringDecoder at the 2 GiB string limit - #37235
Check base64 output length before encoding and cover btoa, StringDecoder at the 2 GiB string limit#37235robobun wants to merge 4 commits into
Conversation
…der at the 2 GiB string limit
|
Status: reproduced the abort on 1.4.0 canary 5b98630 (rc 134, 3/3 for both btoa and StringDecoder base64). Confirmed current main already throws ERR_STRING_TOO_LONG for every encoding through this path after #37215; this PR adds the missing pre-encode length check and regression coverage for these callers. Tests verified failing on a pre-#37215 build and passing here. CI (build 90827, final): 194 of 196 jobs passed, including the new tests on every lane that runs them. The two red jobs are unrelated to this diff:
This diff does not touch module caching, http2, or napi. Review threads are all resolved (two findings fixed, two withdrawn). Ready for a maintainer; I am not pushing further retriggers. |
WalkthroughChangesEncoding length safety
Possibly related PRs
Suggested reviewers: 🚥 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: 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 `@test/js/node/string_decoder/string-decoder.test.js`:
- Around line 426-429: Add the tracked regression issue URL to the comments at
test/js/node/string_decoder/string-decoder.test.js lines 426-429 and
test/js/web/util/atob.test.js lines 73-76, using the same URL in both locations
while preserving their existing explanations.
🪄 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: 8df334e7-6ae5-470a-a853-ecf2fcb5a408
📒 Files selected for processing (3)
src/runtime/webcore/encoding.rstest/js/node/string_decoder/string-decoder.test.jstest/js/web/util/atob.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — the 4-line pre-check in encode_base64_to_bun_string short-circuits before the 2 GiB allocate-encode-discard while preserving the exact observable behavior (Dead → ERR_STRING_TOO_LONG), and the new tests lock in the #37215 fix for the previously-uncovered btoa/StringDecoder callers. One inline nit about gating the atob.test.js timeout on isDebug || isASAN for consistency with the sibling test, but it's not blocking.
Extended reasoning...
Overview
This PR adds a 4-line early-return in encode_base64_to_bun_string (src/runtime/webcore/encoding.rs) that checks the computed base64 output length against BunString::max_length() before allocating and encoding, plus three subprocess-based regression tests covering btoa and StringDecoder#write at the 2 GiB WTF::StringImpl::MaxLength boundary.
Correctness of the production change
I verified against src/bun_core/string/mod.rs that create_external_globally_allocated_latin1 (the constructor the large-output branch calls after encoding) already performs bytes.len() > Self::max_length() and returns String::DEAD. So the new check is a strict prefix of existing behavior — same Dead result, just before the doomed try_reserve_exact(to_len) + full encode. to_len and max_length() are both usize, no signedness or overflow concern. The comment accurately notes the constructors re-check, so this is not the sole guard.
Tests
The tests follow established patterns in the repo: Buffer.alloc(n, fill).toString() instead of .repeat(), concurrent Promise.all pipe draining, stdout asserted before exitCode, os.totalmem() < 10 GiB skip gate matching blob-oom.test.ts, and the JSON.parse(stdout || diagnostic) fallback surfaces stderr/exitCode when the subprocess aborts (so a regression produces a useful failure, not "unexpected end of JSON"). I spot-checked the arithmetic: 1610612734 → ceil(/3)*4 = 2^31 (fails), 1610612733 → 2147483644 (succeeds), 1610612736 → base64 2^31 / hex 3.2e9 (both fail). The string-decoder test reuses the exact isDebug || isASAN ? 60_000 : undefined timeout convention already used twice in that file.
Security risks
None. This adds an upper-bound length check on the encode path; it only makes an already-failing operation fail earlier.
Level of scrutiny
Low-to-medium. The runtime change is 4 lines with no new control flow beyond an early return that mirrors a downstream check. The rest is test coverage for a crash class already fixed in #37215.
Other factors
The one inline nit (unconditional 90s timeout in atob.test.js vs. the gated form used in the sibling change) is a consistency issue only — an ungated higher ceiling is harmless in practice, and the tests are already memory-gated. Not worth blocking on.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?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 `@test/js/web/util/atob.test.js`:
- Line 79: Remove the ad hoc timeout override assigned to timeout in the
subprocess test. Run the test under the repository’s default timeout on debug
and ASAN builds; if it fails, reduce the test workload or use a documented,
measured exception rather than restoring a per-test 90-second timeout.
🪄 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: b742cc82-e006-4e88-a25b-696e12e26b79
📒 Files selected for processing (1)
test/js/web/util/atob.test.js
There was a problem hiding this comment.
LGTM — the timeout gating nit from the earlier pass was addressed in aa975a8, and the comment-cop note in 006877e.
What was reviewed:
- The 5-line
encoding.rschange:to_len(usize) vsBunString::max_length()(usize, clamped toWTF_STRING_MAX_LENGTH) — same observabledead()result as before #37215, just before the allocate+encode instead of after. - Boundary arithmetic in all three test cases (1610612734 → 2^31, 1610612733 → 2147483644, 1610612736 → 2^31 base64 / 3221225472 hex) — all correct.
- Test structure matches the existing 2 GiB blocks in this file and
blob-oom.test.ts/fs-oom.test.ts: sametotalmemgate, subprocess isolation, concurrent pipe drain, exact error code/message assertions,Buffer.alloc(n, fill)over.repeat().
Extended reasoning...
Overview
Three files: a 5-line pre-allocation length check in src/runtime/webcore/encoding.rs::encode_base64_to_bun_string, plus memory-gated subprocess regression tests in test/js/web/util/atob.test.js and test/js/node/string_decoder/string-decoder.test.js covering the 2^31-char output boundary for btoa and StringDecoder base64/hex.
Security risks
None. The Rust change adds a bounds check that makes an already-correct failure path (dead string → ERR_STRING_TOO_LONG, established by #37215) fail earlier — before allocating and encoding ~2 GiB of doomed output. No new surface, no relaxed validation.
Level of scrutiny
Low-to-moderate. The native change is a single usize > usize comparison against BunString::max_length() (verified: returns usize, clamped to WTF_STRING_MAX_LENGTH), with identical observable behavior to the fallthrough path. The tests are the bulk of the diff and follow the exact pattern already present twice in string-decoder.test.js and in blob-oom.test.ts/fs-oom.test.ts (same os.totalmem() < 10 * 1024 ** 3 skip gate appears in 5 files).
Other factors
All prior review threads are resolved: my earlier nit about unconditional vs gated timeout was fixed in aa975a8 (now isDebug || isASAN ? 90_000 : undefined, matching the sibling file); the comment-cop paragraph-length note was addressed in 006877e (comment trimmed to one line); both CodeRabbit findings were withdrawn after author responses. Tests drain stdout/stderr/exited concurrently, assert exact {name, code, message}, cover both sides of the boundary (over → throws, at-limit → succeeds), and use Buffer.alloc(n, 0x61).toString() per repo guidance. The PR description states both new too-long tests were verified failing on a pre-#37215 build.
2 participants
Problem
btoa()andStringDecoder#writewith base64/hex used to abort the process (silent SIGABRT, ~4 GB RSS) when the encoded output reached 2^31 characters, whileBuffer#toString("base64")on the same bytes threwERR_STRING_TOO_LONGcleanly. Debug builds failed with:Repro (rc 134 on 1.4.0 canary 5b98630, 3/3):
Cause
functionBTOAandJSStringDecodercallBun__encoding__toStringdirectly and skipped the output-size pre-checks thatJSBuffer.cpp'stoStringperforms (String::MaxLength, hex x2, base64 4/3). The abort itself was fixed by #37215, which clampedbun_core::String::max_length()toWTF::StringImpl::MaxLength: the encoders now produce a Dead string that surfaces asERR_STRING_TOO_LONG. Verified on current main that btoa and StringDecoder base64/hex/latin1/ascii/utf8 all throw instead of aborting. But none of these callers had test coverage, and the base64 encoder only discovered the overflow after allocating and encoding the full oversized output (2 GiB of doomed work on the btoa path).Fix
encode_base64_to_bun_stringchecks the computed output length againstString::max_length()before allocating, mirroring how the hex path fails fast increate_uninitialized_latin1. Same observable behavior (ERR_STRING_TOO_LONG), minus the 2 GiB allocate-encode-discard. With the check, the failing btoa case drops from ~7s to ~2s under a debug build.blob-oom.test.tssubprocess pattern (gated onos.totalmem() >= 10 GiB):test/js/web/util/atob.test.js: btoa with output 2^31 throwsERR_STRING_TOO_LONG; the largest input whose output fits (2147483644 chars) still encodes.test/js/node/string_decoder/string-decoder.test.js:write()with base64 and hex output over 2^31 - 1 throwsERR_STRING_TOO_LONG. This complements the existing test for oversized utf8 input: here the input buffers are well under the limit and only the encoded output crosses it.Verification
bun bd test test/js/web/util/atob.test.jsandbun bd test test/js/node/string_decoder/string-decoder.test.js: all pass.[review] gate passed · iteration 0 · 3 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