Repository navigation
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (10)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughBrotli and Zstandard initialization failures now use shared cleanup and thrown-error handling. JavaScript wrappers no longer rely on false initialization results. Tests cover error behavior across APIs, stream lifecycle, and reentrant callbacks. Changeszlib initialization error handling
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to No actionable issue remains from this review; the change is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: reworked after a design review. The PR is a draft until a self-review finishes. The measurements are in the PR body. Reproduced on bun 1.4.3-canary.1 (main at 4b02e10), compared against node v26.3.0: const zlib = require("node:zlib");
const dict = Buffer.from([0x37, 0xa4, 0x30, 0xec, 1, 2, 3, 4, 5, 6, 7, 8]); // zstd dictionary magic + junk
zlib.zstdDecompressSync(zlib.zstdCompressSync("hello"), { dictionary: dict });
// main: <Buffer > (no error)
// node 26.3: ERR_ZLIB_INITIALIZATION_FAILED: Failed to load zstd dictionary
// this PR: ERR_ZLIB_INITIALIZATION_FAILED: Failed to load zstd dictionary
Tests are in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, well-scoped Node.js compat fix that routes a previously-dropped zstd init error through the same throw path the sibling set_params failure already used.
What was reviewed:
- Confirmed the Zstd JS constructor (
src/js/node/zlib.ts:790) never checkedhandle.init()'s return, so the oldemit_error+return falsewas silently dropped (onerrorisn't installed untilZlibBaseruns); throwing is the only way to surface it, matching node'sZstdStream::Init. - Checked the sibling
NativeBrotli::initfor the same pattern — it also returnsfalseon init failure, butzlib.ts:736already guards on that and throws$ERR_ZLIB_INITIALIZATION_FAILED, so brotli was never silent; this PR is correctly scoped to zstd. throw_initialization_failedcallsContext::close()which is idempotent whenstateisNone(the state after a dictionary-load failure), so no double-free / null-deref on the new call site.- Tests cover sync/async/stream entry points with exact error code/message and a positive raw-dictionary round-trip control.
Extended reasoning...
Overview
Two files touched. NativeZstd.rs extracts the existing set_params-failure throw block into a private throw_initialization_failed helper and reuses it for the Context::init failure branch, which previously called emit_error (routing through the not-yet-installed onerror callback) and returned JSValue::FALSE. zlib.test.js adds a four-case describe block exercising a malformed formatted dictionary (zstd magic + junk) through zstdDecompressSync, zstdDecompress, and createZstdDecompress, plus a raw-content-dictionary positive control.
Security risks
None. The change converts a silent failure into a thrown ERR_ZLIB_INITIALIZATION_FAILED; no new input is accepted, no new allocation, no new unsafe. The existing SAFETY: invariant on err.msg (non-null when is_error()) holds because every Error::init call in Context::init / load_dictionary passes a C-string literal.
Level of scrutiny
Low-medium. This is a Node.js-compat behavioral fix in a well-tested subsystem. The Rust change is a pure refactor of one branch plus rerouting of another; no memory-management semantics changed (the helper calls Context::close(), which early-returns when state is None, exactly the state the failed-init path leaves it in). The set_params path is byte-for-byte behavior-preserved.
Other factors
The "fix the whole class" question (does NativeBrotli::init share the bug?) was raised and refuted: brotli's emit_error + return false is caught by if (!handle.init(...)) throw $ERR_ZLIB_INITIALIZATION_FAILED() at src/js/node/zlib.ts:736, whereas the Zstd constructor at line 790 never inspected the return value — so only zstd was silently swallowing the failure. Tests assert the exact {name, code, message} shape and verify the async callback is not invoked, and the PR description reports the full zlib.test.js (391 tests) plus the node-parallel zstd/brotli/dictionary suites pass.
A zstd dictionary that fails to load (or a failed ZSTD_createCCtx/DCtx) was reported through the handle's onerror callback, which zlib.ts only installs after init() returns. The error was dropped, the handle kept running with no context, and zstdDecompressSync() returned an empty buffer while createZstdDecompress() produced a stream that emitted nothing. Throw ERR_ZLIB_INITIALIZATION_FAILED with the context's message instead, the same way a failed parameter set already does and the same way node's ZstdStream::Init does.
822da7d to
b83f405
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused Node.js compat fix that makes zstd init failures throw instead of silently returning empty output.
What was reviewed:
- Traced why the old
emit_errorpath was dropped:zlib.tsinstallsonerroronly afterhandle.init()returns, and the Zstd constructor (unlike Brotli's) does not check init's return value. throw_initialization_failed→close_internal:throw_unless_idleearlier ininit()guaranteeswrite_in_progressis false, andContext::close()handles thestate.is_none()case, so the cleanup path is sound.- Confirmed
NativeBrotli::initdoes not share this bug — its JS caller checksif (!handle.init(...))and throws. - Tests cover sync/async/stream + a positive raw-content-dictionary round-trip, and assert the exact error code/message.
Extended reasoning...
Overview
This PR fixes a Node.js compat bug where zlib.zstdDecompressSync / zstdDecompress / createZstdDecompress with a malformed zstd dictionary returned empty output instead of throwing ERR_ZLIB_INITIALIZATION_FAILED. Three files touched: NativeZstd.rs (the fix — replace emit_error with a throwing helper and dedupe the two failure sites), node_zlib_binding.rs (widen close_internal from private to pub(crate)), and zlib.test.js (four new regression tests).
Security risks
None. The change surfaces an already-detected error to the caller instead of dropping it. No new parsing of untrusted input; the dictionary bytes were already being handed to ZSTD_DCtx_loadDictionary, which already rejected them — only the error-reporting path changed. The single unsafe block (cstr(err.msg)) is guarded by err.is_error() at both call sites, which guarantees msg is non-null.
Level of scrutiny
Low-to-moderate. The diff is small (~30 lines net), the root cause is precisely identified in the PR description, and the fix is the same shape Node uses (throw from native init). I verified the concern that close_internal might misbehave during init: throw_unless_idle runs before either failure site so write_in_progress is false, this_value is empty (only set during async write), and Context::close() early-returns on state.is_none(). The refactor of the set_params error path into the same helper is a straight deduplication with no semantic change beyond the harmless extra pending_close.set(false) / this_value.deinit().
Other factors
The bug-hunting system's finder agents flagged the sibling NativeBrotli::init as having the same pattern; verifiers refuted it, and I confirmed why: src/js/node/zlib.ts:736 wraps Brotli's handle.init() in if (!...) throw $ERR_ZLIB_INITIALIZATION_FAILED(), whereas the Zstd call site at line 790 does not check the return value. So the class is fully covered. The comment-cop bot's feedback about long justification comments was addressed in follow-up commits (c7c2adb, 3f3d5c4). Tests assert the exact error name/code/message, use jest.fn() to verify the callback is not invoked, and include a positive round-trip so the change doesn't over-reject valid dictionaries.
|
Updated 7:45 AM PT - Oct 2nd, 2026
✅ @robobun, your commit f9963cf1bd9b36edacaca301c6fba508db6ad8fe passed in 🧪 To try this PR locally: bunx bun-pr 38684That installs a local version of the PR into your bun-38684 --bun |
There was a problem hiding this comment.
LGTM — focused Node compat fix that turns a silent zstd init failure into the same ERR_ZLIB_INITIALIZATION_FAILED throw Node emits.
What was reviewed:
throw_initialization_failed→close_internal: at init timewrite_in_progress/pending_closeare false andthis_valueis empty, so the refactored param-set path is behavior-preserving;Context::close()is a no-op whenstateisNone, so the dictionary-failure path double-close is safe.- Sibling
NativeBrotli::inituses the sameemit_error+return FALSEpattern, butzlib.tschecks the brotli return value and throws — so it is not affected. - Tests cover sync/async/stream constructor, assert the exact error code/message, and include a positive control (raw-content dictionary still round-trips).
Extended reasoning...
Overview
This PR fixes node:zlib zstd streams to throw ERR_ZLIB_INITIALIZATION_FAILED when a formatted zstd dictionary fails to load, instead of silently producing an empty buffer / dead stream. It touches three files: NativeZstd.rs (route both init-time failures through one helper that closes the handle and throws), node_zlib_binding.rs (widen close_internal to pub(crate) so the helper can reuse it), and zlib.test.js (four new tests). The root cause — emit_error firing before zlib.ts installs onerror, so the error is dropped and the JS side ignores the false return — is well-explained and the fix matches Node's ZstdStream::Init behavior of throwing synchronously.
Security risks
None. The change only turns a previously-swallowed error into a thrown one; no new parsing, no new trust surface, no resource-lifetime change beyond earlier cleanup on failure.
Level of scrutiny
Low-to-medium. This is a small, mechanical Node-compat fix in the zstd binding: one emit_error call becomes a throw, and the existing inline throw path for parameter failures is deduplicated into the same helper. I traced close_internal at init time to confirm the refactor is behavior-preserving (no in-flight write, empty StrongOptional, Context::close is idempotent on state = None). I also checked src/js/node/zlib.ts to confirm the Zstd constructor never inspected the old false return (unlike the Brotli constructor, which does — hence the sibling is not affected, matching what the verifier concluded).
Other factors
Tests are strong: they exercise all three user-facing entry points, assert the exact {name, code, message}, verify the async callback is not invoked, and include a positive control so a future over-eager rejection would be caught. The PR description records that the whole 391-test file plus the upstream test-zlib-zstd* and test-zlib-dictionary-fail parallel tests pass, and that the failing cases fail on released bun. The comment-cop feedback about long comments was addressed in follow-up commits (helper now uses close_internal directly with a one-line doc comment). All inline threads are resolved. No human reviewer has left outstanding feedback.
…t fails to load createZstdHandle in internal/streams/iter/transform.ts calls the same NativeZstd.init. Without the fix, decompressZstdSync throws ERR_ZLIB_DICTIONARY_LOAD_FAILED at the first chunk and decompressZstd resolves with zero bytes. node v26.3.0 throws ERR_ZLIB_INITIALIZATION_FAILED from both.
There was a problem hiding this comment.
LGTM — the latest push adds the node:zlib/iter coverage, and the native change is a small, well-contained consolidation of the two init failure branches.
What was reviewed:
throw_initialization_failedrouting teardown throughclose_internal:initalready callsthrow_unless_idle, sowrite_in_progressis false and the handle is fully closed (no deferredpending_closestate);Context::closeis idempotent whenstateisNoneafter a failedload_dictionary, andthis_valueis still an emptyStrongOptionalat that point.- The
unsafecstr(err.msg)read is the pre-existing set_params path moved verbatim;Error::initalways supplies a NUL-terminated static message. - The iter child-process test drains stdout/stderr concurrently, asserts stdout before exitCode, and the async variant builds its handle lazily inside the generator so the rejection (not a sync throw) is the right expectation.
Extended reasoning...
The change touches src/runtime/node/zlib/NativeZstd.rs (two init error branches collapsed into one helper that closes the handle and throws ERR_ZLIB_INITIALIZATION_FAILED), widens close_internal to pub(crate) in node_zlib_binding.rs, and adds a "dictionary that fails to load" block to test/js/node/zlib/zlib.test.js covering sync, callback, stream, and node:zlib/iter entry points plus a raw-dictionary round-trip. No security-sensitive surface is involved; the only unsafe is a relocated, already-justified C-string read. It is a ~30-line native diff with clear intent, the prior-run nit about missing iter coverage was addressed by the last commit, no CODEOWNERS entry covers these files, and the bug hunt ran dry with no findings.
A failed init() on a NativeZstd or NativeBrotli handle now goes through CompressionStream::init_failed: close the handle, call onerror when one is installed, then throw ERR_ZLIB_INITIALIZATION_FAILED. That is the order node's ZstdStream::Init and BrotliCompressionStream::Init use (EmitError, then throw), with the handle closed first so the callback cannot drive a handle that has no context. zstd throws the context's own message. brotli throws node's fixed text 'Initialization failed'. init() returns undefined on success, as in node, so the two Brotli return checks in zlib.ts and internal/streams/iter/transform.ts and the zero-argument ERR_ZLIB_INITIALIZATION_FAILED builtin are gone.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
The node:zlib/iter zstd cases from the earlier round are now covered in zlib.test.js, and beyond the inline nit I checked two things in the new init_failed helper and ruled them out: the onerror-before-throw ordering is harmless for every caller that installs onerror before init (the iter transforms' handlers only set state that the thrown error supersedes, and zlib.ts installs it after init), and the cstr(err.msg) read after close_internal is safe because every Error produced by Brotli/Zstd init/set_params carries a static C literal, not context-owned memory.
Extended reasoning...
The diff moves Brotli and Zstd init failures in src/runtime/node/zlib/ into one Rust helper that closes the handle, fires onerror, and throws ERR_ZLIB_INITIALIZATION_FAILED, and drops the now-dead JS-side $ERR_ZLIB_INITIALIZATION_FAILED() check and its zero-arg builtin; it touches no security-sensitive surface. The Brotli path keeps its "Initialization failed" message and the Zstd set_params path keeps its own message, so observable behaviour is preserved except for the intended zstd dictionary-failure fix, which now has sync, callback, stream and iter coverage. Only a test-precision nit was found, but the change invokes a user callback from native code before throwing and reads a raw C pointer under an unsafe block, so a human maintainer look is still worthwhile.
init_failed takes the thrown message explicitly. Tests cover the doubly-invalid option order, a live stream whose _handle.init() fails, onerror being called once before the throw, re-entrancy from onerror, and the node:zlib/iter brotli caller.
robobun
changed the title
The exit is cold. Inlined, it grew the release init thunks to 2546 B (zstd) and 2669 B (brotli). Out of line they are 2305 B and 2308 B, and release text grows by 428 B instead of 940 B over the merge base.
Problem
zlib.zstdDecompressSync(data, { dictionary })returns an emptyBufferwhen zstd rejects the dictionary. node v26.3.0 throwsERR_ZLIB_INITIALIZATION_FAILED: Failed to load zstd dictionary.NativeZstd::init(src/runtime/node/zlib/NativeZstd.rs:237) reported the failure only throughonerror, whichzlib.tsinstalls afterinit()returns. The handle stayed open with no context, so writes produced nothing.Fix
init()now takes one exit,CompressionStream::init_failed: close the handle, callonerrorif installed, throwERR_ZLIB_INITIALIZATION_FAILED.node_zlib.cc. zstd throws the context message, brotli throws node's fixedInitialization failed.init()returnsundefinedas in node, so both Brotli JS return checks are deleted.test/js/node/zlib/(the released build fails 18 new cases), and the vendoredtest-zlib-*andtest-stream-iter-transform-*files.Background
node:zlibstream is a JSTransformaround a native handle. Its constructor callshandle.init()before it assignshandle.onerror.CompressionStream<T>is the Rust mixin shared by the zlib, brotli and zstd handles.Downsides
_handle.init()now throws. It returnedfalsebefore.init(). node keeps it open, so a secondinit()works in node and throwsERR_INVALID_STATEhere.init()runs 4 more instructions (1635 to 1639). A brotli one runs 3 fewer. Release text grows by 428 B.Notes
What was wrong, per entry point (bun 1.4.3-canary.1+367d939d9, dictionary = zstd magic
0xEC30A437+ junk)zstdDecompressSyncBufferERR_ZLIB_INITIALIZATION_FAILEDzstdDecompress(cb)cb(null, <empty>)cbnever runscreateZstdDecompress,new ZstdDecompressnode:zlib/iterdecompressZstdERR_ZLIB_INITIALIZATION_FAILEDnode:zlib/iterdecompressZstdSyncERR_ZLIB_DICTIONARY_LOAD_FAILED, errno -1, at the first chunkERR_ZLIB_INITIALIZATION_FAILEDwhen the handle is creatednode:zlib/iter(src/js/internal/streams/iter/transform.ts, behind--experimental-stream-iter) builds its own handle and calls the same nativeinit, so it needs no edit.Why the exit is in the mixin
Initat all four zstd and brotli failure sites, afterEmitError(ZstdStream::InitandBrotliCompressionStream::Initinsrc/node_zlib.cc).lib/zlib.jscallshandle.init()with no return check.onerrorruns. With the other order, awriteSync()from insideonerrorcompleted with zero progress on a handle that has no context.Handle-level behavior, same script under node v26.3.0 and this PR (15 of 15 lines equal)
onerrorinstalled first: one call, then the throw. Arguments:('Failed to load zstd dictionary', -1, 'ERR_ZLIB_DICTIONARY_LOAD_FAILED'),('Setting parameter failed', -1, 'ERR_ZSTD_PARAM_SET_FAILED'),('Setting parameter failed', -1, 'ERR_BROTLI_PARAM_SET_FAILED')._handle.init()fails:destroyed === trueand_handle === nullbefore the throw, then'error'and'close'.onerror. That left a live stream open over a closed handle.Not changed here
NativeZlib::inithas no failure return. It latches the handle closed, and node reportsZ_STREAM_ERRORat the first write. node's zlib init is lazy, so that needs its own change.init()the handle is closed (the rule from node:zlib: don't abort when the native handle is driven outside the zlib.ts lifecycle #32762). node keeps the context, soinit()orwriteSync()from insideonerrorsucceeds in node and throwsERR_INVALID_STATEhere.onerrorthat throws is reported asuncaughtException. node swallows it. main does the same today.reset()still reports a failed re-init throughonerroronly, as node does. node:zlib: reset a zstd stream in place, keep its dictionary and parameters #44261 edits that path.ZSTD_error_memory_allocation, in node too. zstd parses a compression dictionary lazily.ArrayBufferdictionary is ignored by thezlib.tsclasses) is separate.Measurements (release builds of merge base 4b02e10 and of this PR, same flags)
size, +428 B).NativeZstdPrototype__init2305 B (was 2112),NativeBrotliPrototype__init2308 B (was 2280),init_failedzstd/brotli 261/506 B (nm -S).stepi, merge base vs PR,createZstdDecompress()andcreateBrotliDecompress()). The compress handles give 4986 vs 4990 and 865 vs 862. The design expected equal counts. The zstd thunk now keepsthislive across the parameter loop for theonerrorcall, which costs 4 stack reloads.llvm-objdump, position-dependent operands normalized).createBrotliHandle99 -> 93.zlib.js26776 B (was 26818),transform.js17667 B (was 17709).BunString__createUTF8ForJScalls under gdb.BUN_JSC_validateExceptionChecks=1 BUN_JSC_dumpSimulatedThrows=1, debug ASAN build): 0 violations. 481 of 483 tests pass in the combined run of the 3 zlib test files. The 2 failures are 5 s timeouts ofNativeZlibgrowable-SharedArrayBufferwrite tests, which do not reachinit_failed. Both pass when run alone with the same flags.node:zlibdecompress forms, 2node:zlib/itertransforms, 4 compress forms and 8 doubly-invalid option sets. Line 20 is apledgedSrcSize: -1check that this PR does not touch.init()with a rejected parameter,false-> throw). The zstd sibling case now also prints theinit()outcome, with no behavior change.[review] gate passed · iteration 3 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 3
evidence per changed file