zlib: accept zlib flush constants in brotli set_flush instead of panicking - #32358
zlib: accept zlib flush constants in brotli set_flush instead of panicking#32358robobun wants to merge 5 commits into
Conversation
…cking The shared CompressionStream write()/writeSync() validation accepts flush values in the zlib range 0..=6 for all of NativeZlib/NativeBrotli/NativeZstd, so Z_FINISH (4) and Z_BLOCK (5) can reach the brotli Context::set_flush. The Rust port mapped only 0..=3 and hit unreachable!() on the rest, aborting the process after write_in_progress/ref_/buffer-pinning state had already been set up. Node stores the raw int: the brotli decoder never reads the flush value at all (BrotliDecoderDecompressStream takes no op argument) and get_error_info only compares it against BROTLI_OPERATION_FINISH, while the encoder treats any unknown op as a non-flushing, non-terminal step. Map values outside the brotli operation set to BROTLI_OPERATION_PROCESS so both paths match Node's observable behavior instead of crashing.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesBrotli flush fallback fix and tests
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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/zlib/zlib.test.js`:
- Around line 354-357: The subprocess test is currently asserting stdout and
exitCode together in a single expect statement. Split this into separate expect
calls where stdout is asserted first using expect(stdout.trim()).toEqual(...)
and exitCode is asserted last using expect(exitCode).toEqual(0). Apply this same
refactoring pattern at both the main location and the second affected location
mentioned in the comment to ensure consistent assertion ordering throughout the
test file.
🪄 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: e337103a-f08f-4f43-9738-51b7a7120b9b
📒 Files selected for processing (2)
src/runtime/node/zlib/NativeBrotli.rstest/js/node/zlib/zlib.test.js
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Related: #31505 and #31996 also address this crash, with a different design. #31505 adds a per-codec typed This PR instead maps out-of-range values to Either approach fixes the abort. Happy to close this if the typed-validation approach in #31505 is preferred. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Updated 6:14 PM PT - Jun 15th, 2026
❌ @robobun, your commit aec9371 has some failures in 🧪 To try this PR locally: bunx bun-pr 32358That installs a local version of the PR into your bun-32358 --bun |
There was a problem hiding this comment.
LGTM — one-line panic fix with thorough tests; all prior feedback addressed.
Extended reasoning...
Overview
The PR replaces a single unreachable!() arm in Context::set_flush (src/runtime/node/zlib/NativeBrotli.rs) with a fallback to Op::process, so zlib flush constants Z_FINISH (4), Z_BLOCK (5), and Z_TREES (6) — which the shared flush_value_is_valid() already lets through — no longer abort the process. The accompanying comment explains the semantics for both encode and decode paths. Four subprocess tests in test/js/node/zlib/zlib.test.js cover BrotliDecompress.flush(Z_FINISH|Z_BLOCK), a decompress round-trip across the flush, and a BrotliCompress encoder round-trip.
Security risks
None. The change narrows behavior from a process abort to a no-op flush operation. Op::process is already the default value of self.flush and is fed to BrotliEncoderCompressStream on every ordinary write, so no new code path or capability is introduced. Inputs are still bounded to 0..=6 by upstream validation.
Level of scrutiny
Low-to-moderate. The functional diff is one match arm plus a comment; the rest is test additions. The reasoning in the PR description is correct against the surrounding code: the decoder never reads self.flush except to compare against Op::finish in get_error_info, and the encoder treats process as a non-flushing step. This is a strictly-better outcome than panicking after write_in_progress/refcount/buffer-pin side effects have already run.
Other factors
All three rounds of review feedback have been incorporated: stdout-before-exitCode assertion ordering (5328345), the BrotliCompress encoder-path test I asked for (e22acc8), and it.concurrent for the subprocess suite (b836da5). No bugs were found by the bug-hunting pass. There are competing PRs (#31505, #31996) that take a typed-validation approach instead; the author has flagged this and offered to defer, but that is a merge-coordination question for maintainers — the code in this PR is independently correct and minimal.
|
CI status: the diff is green on all completed lanes. The new Remaining red/pending is unrelated to this change:
None of these touch |
…lib.ts lifecycle (#32762) ### What The `zlib.ts` wrapper drives the `NativeZlib` / `NativeBrotli` / `NativeZstd` handles as constructor -> `init()` -> `write*()` -> `close()`, caches `onerror` and the write callback in `init()`, and nulls `_handle` on close. The native binding assumed that protocol. Driving a handle outside it, which only takes reaching `_handle` (and its `constructor`), aborts the whole process. Eleven distinct ways, all reproduced on the release binary; two are null-pointer segfaults and one needs nothing but `new`: ```js const zlib = require("node:zlib"); // 1) 2^32-byte dictionary via the PUBLIC API (no _handle needed): zlib.deflateSync(Buffer.from("hi"), { dictionary: new Uint8Array(2 ** 32) }); // panic: int cast: TryFromIntError(PosOverflow) // 2) constructing a handle and never initializing it; the GC finalizer // frees/resets native state that was never created: new (zlib.createBrotliCompress()._handle.constructor)(8); Bun.gc(true); // panic: called `Option::unwrap()` on a `None` value new (zlib.createZstdCompress()._handle.constructor)(10); Bun.gc(true); // panic(main thread): Segmentation fault at address 0xE38 // 3) writeSync() before init() (brotli/zstd): NULL state pointer into the C library new (zlib.createZstdCompress()._handle.constructor)(10).writeSync(0, null, 0, 0, new Uint8Array(64), 0, 64); // panic(main thread): Segmentation fault at address 0xE38 // 4) writeSync() after close() (brotli/zstd): const h = zlib.createBrotliCompress()._handle; h.close(); h.writeSync(0, null, 0, 0, new Uint8Array(64), 0, 64); // panic: internal error: entered unreachable code // 5) init() after close() (all three): const h2 = zlib.createDeflate()._handle; h2.close(); h2.init(15, 6, 8, 0, new Uint32Array(2), () => {}, undefined); // panic: internal error: entered unreachable code // 6) init() whose deflateInit2_ rejects the arguments, then init() again: const h3 = new (zlib.createDeflate()._handle.constructor)(1); h3.init(100, 6, 8, 0, new Uint32Array(2), () => {}, undefined); h3.init(15, 6, 8, 0, new Uint32Array(2), () => {}, undefined); // panic: internal error: entered unreachable code // 7) any stream error with no cached onerror: new (zlib.createDeflate()._handle.constructor)(1).reset(); // panic: Assertion failure: cachedErrorCallback is null in node:zlib binding // 8) an async write completing with no cached writeCallback: const h4 = new (zlib.createBrotliDecompress()._handle.constructor)(9); h4.reset(); h4.write(0, null, 0, 0, new Uint8Array(64), 0, 64); // panic: called `Option::unwrap()` on a `None` value ``` Plus two that are not aborts but are worse: `init()` or `params()` called while an async `write()` is still running on the thread pool mutates the same native `z_stream` / encoder from two threads (the worker holds `&mut Context` inside `deflate()` while the JS thread runs `deflateInit2_` on it), and a brotli/zstd `init()` whose parameter setup fails tears the `Context` down without marking the handle closed, so the next call aborts anyway. ### Why - After `close()`, or after a failed `init()`, the `Context` is in `NodeMode::NONE` with its native state freed; `Context::{do_work,init}` and brotli's `get_error_info` have no arm for `NONE` and hit `unreachable!()`. `reset()` already guarded on `closed`; `write`, `writeSync` and `init` did not, and nothing guarded against an in-flight write. - `init()` can fail three different ways without anything recording it: zlib's `deflateInit2_`/`inflateInit2_` rejecting the arguments, and brotli/zstd's per-parameter setup failing after the state was created. All three leave `{ closed: false, mode: NONE }`. - Before `init()`, brotli/zstd have no encoder/decoder state at all. `do_work()` passes the null pointer into `BrotliEncoderCompressStream` / `ZSTD_compressStream2`, and the GC finalizer does the same through `Context::close()` (`deinit_state` unwraps the `None`; zstd hands the null `CCtx` to `ZSTD_CCtx_reset`). So a handle that is merely constructed and collected already aborts. - `NativeZlib::Context::close()` asserted that `deflateEnd`/`inflateEnd` returned `Z_OK`/`Z_DATA_ERROR`; on a `z_stream` whose init never ran they return `Z_STREAM_ERROR` (with nothing to free), which fired the assertion in debug/ASAN builds. - `emit_error()` and the async write-completion path `unwrap()` JS callbacks that only `init()` caches. - `NativeZlib::init()` copied the dictionary into a `Vec` and only then did `u32::try_from(dict.len()).expect("int cast")`; JSC allows a 2^32-byte `Uint8Array` and `deflateSetDictionary` takes a `uInt` length. ### Fix - `throw_if_closed` is generalized into `CompressionStream::throw_unless_idle` (`write_in_progress` / `pending_close` / `closed`, in the shared mixin so all three stream types at once). `write()`, `writeSync()`, each class's `init()`, and `NativeZlib::params()` call it; `write()`/`writeSync()` previously carried their own copies of the first two checks. - Every `init()` failure path now marks the handle `closed`: `NativeZlib::init()` checks whether `Context::init` ended in `NodeMode::NONE`, and the brotli/zstd parameter-failure branches set it after tearing the `Context` down. - Brotli and zstd `Context::close()` early-return when the native state was never created. This also makes `close()` idempotent, which overlaps the user-called double-`close()` case that #32382 fixes in the same function; it became required here because the GC finalizer goes through the same `close()`. - `NativeZlib::Context::close()`'s debug assertion also accepts `Z_STREAM_ERROR`. - Brotli/Zstd `Context::do_work()` return early when the state was never created. `NativeZlib` already tolerates this (zlib's own `deflateStateCheck` null-checks the stream). - `emit_error()` and the write-completion path skip the JS callback when none is cached instead of panicking; the pending reset/close handling still runs. Node behaves the same way. - `NativeZlib::init()` rejects a dictionary longer than `u32::MAX` before copying it: `RangeError [ERR_OUT_OF_RANGE]: The value of "dictionary.byteLength" is out of range. It must be <= 4294967295. Received 4294967296`. With the check before the copy, a 2^32-byte `Uint8Array` argument is never read, so the test stays cheap. Intentionally not touched here: - brotli `set_flush` receiving zlib flush constants `4..=6` is #32358 (whose fix, mapping them like Node does, is the right one). - a second `init()` on an already-initialized, idle handle leaks the first encoder/`internal_state`. That is a bounded leak rather than an abort, Node has the same gap, and it needs its own leak-regression test; left as a follow-up. - `NativeZlib::params()` after `close()` is already safe (`Context::set_params` has a `_ => {}` arm). ### Test `test/js/node/zlib/zlib-handle-bounds-check.test.ts` gains one subprocess test per case (21 new). The children run with `BUN_DESTRUCT_VM_ON_EXIT=1` so the GC finalizers run deterministically on every lane, not only the ASAN lanes that destroy the VM at exit (which is how CI caught the finalizer class in the first place). ``` bun bd test test/js/node/zlib/zlib-handle-bounds-check.test.ts # 30 pass bun bd test test/js/node/zlib/zlib.test.js # 376 pass (2 pre-existing debug-only timeouts) USE_SYSTEM_BUN=1 bun test <same file> # the new cases fail (aborts / wrong result) ``` The 14 `test-zlib-{write-after-close,close-after-*,flush*,brotli*,params,dictionary*,reset-before-write,...}` node-upstream tests still pass. The two `streaming encode doesn't wait for entire input` failures in `zlib.test.js` are pre-existing on debug+ASAN runners: the test's own byte-fill setup loop takes 42s against a 15s limit before any compression happens.
|
Closing in favor of #31505. Both fix the same crash: |
1 participant
Reproduction
Node.js prints
flushedfollowed byerr: Z_BUF_ERROR. Current Bun aborts with:Cause
CompressionStream::write/write_syncinsrc/runtime/node/node_zlib_binding.rsvalidate the flush argument with a sharedflush_value_is_valid()that accepts the zlib range0..=6for all threeNative{Zlib,Brotli,Zstd}classes.Z_FINISH(4) andZ_BLOCK(5) therefore pass validation and reach brotli'sContext::set_flush, whose Rust port only mapped0..=3(theBrotliEncoderOperationdiscriminants) and hitunreachable!()on the rest. The Zig reference used@enumFromInt(flush)which stored the value without trapping.The panic fires after
write_in_progress = true,ref_(), and buffer pinning have already run, so even if it were caught the pinned buffers and refcount would leak.Fix
Map flush values outside
0..=3toBROTLI_OPERATION_PROCESS. ForBROTLI_DECODEthe flush value is never passed to the C decoder (BrotliDecoderDecompressStreamtakes no op argument) andget_error_infoonly compares it againstBROTLI_OPERATION_FINISH(2), so any non-2 value is equivalent. ForBROTLI_ENCODEthe C encoder treats any op outside its switch arms as a non-flushing, non-terminal step, whichPROCESSalready is.Verification
Added subprocess tests in
test/js/node/zlib/zlib.test.jscoveringZ_FINISHandZ_BLOCKonBrotliDecompress, asserting the flush callback fires andend()surfacesZ_BUF_ERROR(matching Node), plus a round-trip test that decodes valid brotli input across aflush(Z_FINISH). All three fail on main (process aborts with exit code 132) and pass with the fix.