node:zlib: accept zstd pledgedSrcSize values of 4 GiB and larger - #33442
node:zlib: accept zstd pledgedSrcSize values of 4 GiB and larger#33442robobun wants to merge 4 commits into
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis change modifies pledgedSrcSize validation in NativeZstd::init to use integer range checking against MAX_SAFE_INTEGER instead of the prior validate_uint32 conversion path, converting to u64 via try_from. A new test file verifies frame header encoding and error handling for invalid inputs. ChangesZstd pledgedSrcSize Validation
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 10:30 AM PT - Jul 6th, 2026
❌ @robobun, your commit 0fb6e2c has 1 failures in
🧪 To try this PR locally: bunx bun-pr 33442That installs a local version of the PR into your bun-33442 --bun |
![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/node/zlib/zstd-pledged-src-size.test.ts`:
- Around line 47-58: The streaming Zstd test only rejects on error and only
destroys the encoder on success, so a premature close can hang the test and a
failed path can leak the native handle. Update the test around
createZstdCompress, Promise.withResolvers, and encoder.flush so every failure
signal (at least error and close, plus any other relevant abort/exit path)
rejects the promise, and ensure encoder cleanup is registered before awaiting so
it runs on both success and failure, not just after the assertions.
- Around line 60-69: The rejection test in zstd-pledged-src-size should be split
into `test.each` cases instead of a manual loop so each invalid input reports
independently and matches repo style. Add `NaN` to the invalid `pledgedSrcSize`
matrix and keep the same `createZstdCompress` assertion shape, using the
`zlib.createZstdCompress` path to verify all non-negative safe integer
violations are covered explicitly.
🪄 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: 15ff19ee-e13c-4133-a4a4-4911e4db48a3
📒 Files selected for processing (2)
src/runtime/node/zlib/NativeZstd.rstest/js/node/zlib/zstd-pledged-src-size.test.ts
|
Addressed both review comments in 938b18b:
Still 3 fail on 1.4.0 / 9 pass with the fix. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — straightforward validator widening from uint32 to safe-integer range for a value the downstream code already handles as u64.
Extended reasoning...
Overview
This PR changes a single validation call in src/runtime/node/zlib/NativeZstd.rs: pledgedSrcSize was previously validated with validate_uint32 (capped at 2³²−1) and is now validated with validate_integer(min=0, max=MAX_SAFE_INTEGER). Everything downstream — Context.pledged_src_size: u64 and ZSTD_CCtx_setPledgedSrcSize(u64) — was already 64-bit, so this is purely relaxing an artificially tight input gate to match Node.js and the zstd frame format (which supports an 8-byte content size field). The u64::try_from(i64) is provably infallible given the validated [0, 2⁵³−1] range.
A new test file covers the sync/async/streaming entry points, verifies the pledged size actually lands in the zstd frame header (not just that no exception is thrown), checks both sides of the old and new boundaries, and asserts ERR_OUT_OF_RANGE for negative/fractional/Infinity/NaN/beyond-safe-integer inputs via it.each.
Security risks
None. pledgedSrcSize is advisory metadata written into the zstd frame header; it does not size any allocation in Bun or in the vendored zstd on the compress path. Widening the accepted range to what the underlying C API natively takes (unsigned long long) introduces no new attack surface.
Level of scrutiny
Low. This is a ~10-line Node-compat fix swapping one existing in-tree validator for another with a wider range. It uses established validators::validate_integer machinery, doesn't touch memory management, threading, or FFI lifetime, and the change is mechanically verifiable against the validator's implementation.
Other factors
Both CodeRabbit inline comments (encoder cleanup in try/finally + wire close to reject; use test.each and add NaN) were addressed in commit 938b18b and are marked resolved. The PR description shows the test failing on the released build and passing on the debug build, and lists the related upstream Node parallel tests as still passing. No bugs were flagged by the bug-hunting system.
The option was validated as a uint32, so pledging a source size of 2**32 or more threw ERR_OUT_OF_RANGE even though ZSTD_CCtx_setPledgedSrcSize takes a u64 and the frame header's content-size field holds 8 bytes. Validate it as a non-negative safe integer instead and pass it through unmodified.
Wire close to the rejection path and destroy the encoder in a finally, so a premature close fails with a diagnostic instead of hanging and the handle is released on both paths. Split the rejected values into it.each cases and add NaN, which only the native validator rejects.
938b18b to
ac2502b
Compare
There was a problem hiding this comment.
LGTM — straightforward validator swap that lifts the artificial 4 GiB cap on pledgedSrcSize, with thorough header-round-trip tests.
Extended reasoning...
Overview
This PR touches two files: a ~10-line change in src/runtime/node/zlib/NativeZstd.rs that swaps validate_uint32 for validate_integer(min=0, max=MAX_SAFE_INTEGER) when parsing the pledgedSrcSize option in NativeZstd::init, and a new 72-line test file test/js/node/zlib/zstd-pledged-src-size.test.ts. Everything downstream of the validator was already 64-bit (Context.pledged_src_size: u64, ZSTD_CCtx_setPledgedSrcSize takes a c_ulonglong, and the zstd frame header's Frame_Content_Size field is up to 8 bytes), so the u32 cap was purely an artifact of the wrong validator being used.
Security risks
None. pledgedSrcSize is metadata written into the zstd frame header — it does not drive any allocation in Bun. Widening the accepted range from [0, 2^32-1] to [0, 2^53-1] cannot cause overflow (validate_integer returns an i64 in [0, MAX_SAFE_INTEGER], and u64::try_from on a non-negative i64 is infallible, as the .expect() correctly notes). No auth, crypto, permissions, or untrusted-input parsing beyond the existing numeric validation, which is strictly tighter than Node's (Node only rejects negatives).
Level of scrutiny
Low. This is a focused Node-compat fix in an option-parsing path with no control-flow changes: same is_number() gate preserving the undefined → ZSTD_CONTENTSIZE_UNKNOWN default, same error-handling structure, just a wider validator. The validate_integer helper is the established in-tree validator used throughout src/runtime/node/ for exactly this case. No memory management, GC, threading, or FFI lifetime changes.
Other factors
The test coverage is unusually thorough for a change this size: it decodes the zstd frame header per the spec and asserts the pledged size actually lands in Frame_Content_Size unmodified (rather than just "didn't throw"), across sync/async/streaming APIs, at the old boundary (2^32-1), just past it (2^32), and at MAX_SAFE_INTEGER. Rejection cases use it.each over MAX_SAFE_INTEGER+1, -1, 1.5, Infinity, and NaN. Both CodeRabbit review comments (try/finally cleanup + close wiring in the streaming test; test.each + NaN case) were addressed and the threads are resolved. The bug-hunting system found no issues. The PR description confirms 3 fail on 1.4.0 / 9 pass with the fix, and that the existing zstd/zlib test suites still pass.
There was a problem hiding this comment.
LGTM — clean validator swap with thorough frame-header verification tests.
Extended reasoning...
Overview
This PR widens the accepted range for zstd's pledgedSrcSize option from [0, 2^32-1] to [0, Number.MAX_SAFE_INTEGER], matching Node.js behavior. The native change in src/runtime/node/zlib/NativeZstd.rs is 8 lines: swap validators::validate_uint32 for validators::validate_integer with explicit Some(0)/Some(jsc::MAX_SAFE_INTEGER) bounds, then u64::try_from the resulting i64. Everything downstream (Context.pledged_src_size: u64, ZSTD_CCtx_setPledgedSrcSize taking c_ulonglong) was already 64-bit, so the old validator was the only bottleneck. A new 72-line test file covers sync/async/streaming entry points and rejection cases.
Security risks
None. pledgedSrcSize is a hint written into the zstd frame header — it doesn't size any allocation or buffer in Bun's code. Widening the accepted range from 32-bit to 53-bit for a value that's already stored and passed as 64-bit introduces no overflow or resource-exhaustion vector. Invalid inputs (negative, non-integer, NaN, >MAX_SAFE_INTEGER) still throw ERR_OUT_OF_RANGE via the existing validator machinery.
Level of scrutiny
Low-to-moderate. This is a targeted Node.js-compat fix in the input-validation layer with no changes to the compression logic itself. I verified validate_integer returns JsResult<i64> and enforces the passed bounds, so u64::try_from on a value in [0, 2^53-1] is genuinely infallible as the .expect claims. The is_number() gate before the validator preserves the existing "undefined → ZSTD_CONTENTSIZE_UNKNOWN" default.
Other factors
The test quality is high: rather than just asserting nothing throws, it decodes the zstd frame header per the spec and checks the pledged size actually landed in Frame_Content_Size, across all three API surfaces. The ZSTD_e_flush trick to skip zstd's pledged-vs-actual check (so 4 GiB can be pledged without feeding 4 GiB) is well-documented in the test. Both CodeRabbit suggestions (wire close to reject + try/finally cleanup; it.each + NaN case) were addressed and are visible in the final diff. The PR description confirms before/after test results and that adjacent zlib/zstd tests still pass. No bugs were found by the bug-hunting system.
|
The red lanes on this PR are not from this diff. Summary for whoever picks it up, updated now that the build has finished. Final result of the latest build: 144 test-bun lanes pass. Every failure is a darwin aarch64 lane, and none of them is a test failing:
Previously red, now green: two earlier failures, both confirmed unrelated and both gone:
The diff is green. Every build lane passes, and Not pushing again, since only a push re-runs CI and there is nothing left to change. Happy to rebase or re-run if that helps. |
1 participant
What's broken
pledgedSrcSizeis the option you reach for when streaming a file too large to buffer, but pledging 4 GiB or more throws:Same error from
zstdCompressSync,zstdCompress, andnode:zlib/iter'scompressZstd. Node accepts it: it only rejects negative values, and writes the pledge straight into the frame header.Cause
NativeZstd.initvalidated the option withvalidate_uint32, capping it at2**32 - 1. Everything downstream of that validator is already 64-bit:Context.pledged_src_sizeis au64, andZSTD_CCtx_setPledgedSrcSizetakes ac_ulonglong. The zstd frame header'sFrame_Content_Sizefield is up to 8 bytes, so sizes above 4 GiB are representable, they just never made it past the validator.Fix
Validate it as a non-negative safe integer (
0 ..= Number.MAX_SAFE_INTEGER, which is the largest integer a JS number holds exactly) and pass the value through unmodified.Verification
test/js/node/zlib/zstd-pledged-src-size.test.tspledges 4 GiB through all three compression APIs and reads the frame content size back out of the header, so it checks the value arrives at zstd unmodified rather than just that nothing threw. Finishing withZSTD_e_flushinstead ofZSTD_e_endskips zstd's pledged-vs-actual check, which is what lets the test pledge 4 GiB without feeding it 4 GiB.Values below the old cap produce byte-identical output to before (and to node),
Number.MAX_SAFE_INTEGERis accepted, and one past it, negatives, non-integers andNaNstill throwERR_OUT_OF_RANGE.before / after
test/js/node/test/parallel/test-zlib-zstd-pledged-src-size.js,test/js/node/test/parallel/test-stream-iter-validation.js,test/js/node/zlib/zlib.test.jsandtest/regression/issue/23314/all still pass.