zlib: make brotli/zstd native handle close() idempotent - #32382
Conversation
Calling _handle.close() twice on a BrotliCompress/BrotliDecompress or
ZstdCompress/ZstdDecompress stream aborted the process with
"panic: internal error: entered unreachable code".
The first close() frees the native encoder/decoder state and sets
Context.mode to NONE. A second close() then entered Context::deinit_state()
(brotli) or the ZSTD_*Ctx_reset match (zstd) with mode == NONE, falling
through to the catch-all unreachable!() arm.
NativeZlib already handles this with an explicit NONE => {} arm. Make the
brotli and zstd Context::close() methods idempotent the same way reset()
already is: only touch the native state when state.is_some(). This also
makes close() before init() a safe no-op.
|
Updated 2:05 AM PT - Jun 16th, 2026
✅ @robobun, your commit 3b322391b9f35acfe3282c84aca88658da5ec341 passed in 🧪 To try this PR locally: bunx bun-pr 32382That installs a local version of the PR into your bun-32382 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
The This PR is the minimal standalone crash fix plus a regression test for the double- |
|
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 (3)
Walkthrough
ChangesIdempotent close() for native zlib handles
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — minimal idempotency guard mirroring the existing reset() pattern, with subprocess regression tests.
Extended reasoning...
Overview
This PR fixes a process-aborting panic when _handle.close() is called more than once on brotli or zstd zlib streams. It touches two Rust files (NativeBrotli.rs, NativeZstd.rs), wrapping the body of Context::close() in an if self.state.is_some() { … } guard so that a second close (where mode == NONE and state == None) becomes a no-op instead of falling into the _ => unreachable!() arm. A new subprocess-based test file exercises double/triple close across all six stream factories.
Security risks
None. The change is purely defensive — it removes a reachable unreachable!() panic path and adds no new FFI calls, no new pointer dereferences, and no new user-controlled inputs. The guarded block still upholds the invariant that state.is_some() implies mode is a valid encoder/decoder mode (set together in init(), cleared together in close()/deinit_state()).
Level of scrutiny
Low-to-moderate. The diff is tiny (one if wrapper per file), mechanically mirrors the existing reset() guard in the same impl blocks, and matches how NativeZlib::Context::close() already handles NONE. The PR description includes a clear repro, root-cause analysis (including why the Zig sibling didn't crash in ReleaseFast), and before/after test results.
Other factors
No CODEOWNERS cover these paths, no prior reviewer comments are outstanding, and the bug-hunting system found no issues. The new test runs each case in a child process so a regression would surface as a non-zero exit rather than killing the test runner. I verified close_internal() in node_zlib_binding.rs indeed does not gate on closed, confirming the described failure path.
…never-initialized one The ASAN CI lane tears the VM down at exit, running every GC finalizer. Once write/reset/init stop aborting early, a handle whose native state was never created reaches the finalizer, which had its own aborts: - Brotli/Zstd Context::close() on a never-created `state`: brotli's deinit_state unwraps the `None`; zstd hands the null CCtx straight to ZSTD_CCtx_reset. Both now early-return. This also makes close() idempotent (the user-called double-close #32382 fixes). - NativeZlib Context::close() asserted that deflateEnd accepted the stream; on a z_stream whose init never ran it returns Z_STREAM_ERROR with nothing to free, so the assertion allows it. Two more gaps in the same surface from review: - init() (all three classes) and NativeZlib::params() only rejected a *closed* handle, so calling them while an async write() still held the Context on a worker thread mutated the same native stream from two threads. `throw_if_closed` becomes `throw_unless_idle` (write_in_progress / pending_close / closed); write()/writeSync() use it too instead of carrying their own copies of the first two checks. - A brotli/zstd init() whose parameter setup fails tears the Context down without marking the handle closed, so the next writeSync()/init() still aborted. That path now sets `closed`. The test children run with BUN_DESTRUCT_VM_ON_EXIT=1 so every finalizer runs deterministically, not only on the lanes that destroy the VM.
…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.
|
Superseded by #32762 (merged as aaba8f0), which applies the same Confirmed on current main: both |
1 participant
Repro
Same crash for
createBrotliDecompress,createZstdCompress,createZstdDecompress.createGzip/createDeflateare unaffected.Cause
CompressionStream::close_internal()does not gate onclosed, so a second JS_handle.close()callsContext::close()again. For brotli and zstd,Context::close()unconditionally enters amatch self.mode { … _ => unreachable!() }(directly in zstd, viadeinit_state()in brotli). After the first close,mode == NONE, so the second call hitsunreachable!()and aborts.NativeZlib::Context::close()already handles this with an explicitNONE => {}arm. The Zig brotli sibling has the sameelse => unreachablebut shipped in ReleaseFast where Zigunreachablecompiles to nothing and execution happened to fall through harmlessly; the Rustunreachable!()panics in every profile.Fix
Guard the native-state teardown in
Context::close()onself.state.is_some(), mirroring howContext::reset()already guardsdeinit_state(). This makesclose()a no-op when already closed (or wheninit()was never called), matching NativeZlib and Node's smart-pointer reset semantics.Verification