Repository navigation
Conversation
`options.rejectGarbageAfterEnd` was accepted and ignored. As in Node.js v26.12.1: - ZlibBase validates the option as a boolean and stores it. - processChunkSync and processCallback raise ERR_TRAILING_JUNK_AFTER_STREAM_END when a native write leaves input and has output space. Every decoder goes through one of the two. - NativeZlib takes the flag at init. With the flag it does not read a second gzip member. No result changes for a caller that does not set the option. The zstd decoder keeps no frame state across writes yet. With the option it refuses what one write leaves. Bytes that come in a later write still give ZSTD_error_prefix_unknown, and 1 to 3 bytes of a frame magic number are still accepted.
|
Status Reproduced on main: With this PR (#47369) all 10 forms throw that error. CI (build 135313, last commit): 186 of 188 jobs ran, and all 186 passed. These are all builds, all Linux, Windows and macOS aarch64 test lanes with the ASAN lane, and 2 of the 4 macOS x64 test jobs. The other 2 macOS x64 test jobs never ran. They waited for an agent longer than BuildKite allows and expired 4 times, and that is why the build shows as failed. A retry of these 2 jobs completes the build. The new test file is in one of them, so macOS x64 is the one platform where it did not run yet. No failure comes from this change. |
|
Updated 1:26 PM PT - Oct 29th, 2026
❌ @robobun, your commit 36eab97 has some failures in 🧪 To try this PR locally: bunx bun-pr 47369That installs a local version of the PR into your bun-47369 --bun |
ZlibBase read the option in a condition and again in the body of that condition. The lint rule no-duplicate-conditional-property-access does not allow that. ZlibBase now reads the option into a local, validates the local and stores it.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 1 minute. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Problem
node:zlibignoresoptions.rejectGarbageAfterEnd:zlib.inflateSync(Buffer.concat([deflated, junk]), { rejectGarbageAfterEnd: true })returns the data. Node.js v26.12.1 throwsERR_TRAILING_JUNK_AFTER_STREAM_END.ZlibBase(src/js/node/zlib.ts:246) only passes it toTransform.Fix
lib/zlib.js,ZlibBasevalidates and stores the option, and both drivers raise the error when a write leaves input.NativeZlibgets the flag atinitand reads no second gzip member.node:test), and without the option no result changes (0 of 798 matrix cases).test/js/node/zlib/reject-garbage-after-end.test.ts(124 pass, main fails 80). Alsotest/js/node/zlib/and 68 vendored Node files.Background
gunzipSyncof two members still returns both. Node's zstd frame state changes a zstd default and conflicts with Replace the zstd C library with a port to safe Rust #47079, so node:zlib: zstd decoder follows Node v26.10.0 across writes and at the end of the input #33413 keeps it.Downsides
ZSTD_error_prefix_unknown, and 1 to 3 bytes of a frame magic pass..text256, built-in JS 590), one 4,096-byte page on disk.finishFlushstops early (Z_BLOCK).Notes
What Node.js v26.12.1 does
The option is in three places of Node:
lib/zlib.js:ZlibBasevalidates it and stores_rejectGarbageAfterEnd.processChunkSyncthrows after its loop.processCallbackdestroys the stream with the error.src/node_zlib.cc,ZlibContext: with the flag, it does not go on to a second gzip member.src/node_zlib.cc,ZstdDecompressContext: it keeps the state of the frames across writes and reports trailing input itself.This PR has the first two. Node added the option in v26.5.0 (nodejs/node#64023).
Results
The script of the report runs
inflate,inflateRaw,unzip,brotliDecompressandzstdDecompress, each in the sync and the callback form, on valid input withJUNKafter it.A matrix of 1327 cases ran on main, on this PR and on Node.js v26.12.1: 7 decoders, the option unset,
trueandfalse, the sync, callback and stream forms, and 21 inputs.The matrix ran on the first commit of this PR. The second commit changes only how
ZlibBasereads the option.ZSTD_error_prefix_unknown. 5 return the data.bytesWrittendiffers from Node, because zlib-ng and Node's zlib give compressed data of different length.zstd with the option set
ZSTD_error_prefix_unknownZ_BUF_ERROR, with or without the optiontodowith Node's result for each open row, and one live test with the result of today.ZSTD_error_prefix_unknownon main, and Node ignores them since v26.10.0 (zlib: improve zstd decoding across chunk boundaries nodejs/node#65865).NativeZstd.rs. Itstest/js/bun/util/zstd.test.tsexpectsZSTD_error_prefix_unknownfor that default. node:zlib: zstd decoder follows Node v26.10.0 across writes and at the end of the input #33413 has the frame state. So the zstd half is not in this PR.test-zlib-reject-garbage-after-end.js12 of 12. It changed 32 of the 798 cases without the option, all zstd.Cost for a caller that does not set the option
Measured on release builds of the merge base (da867c6) and of this PR, linux-x64.
Each native write:
Each stream (a one-shot call makes one stream):
_rejectGarbageAfterEnd, as in Node: 23 to 24 keys for a zlib class, 21 to 22 for brotli and zstd.ZlibBase6 more with an options object and 4 more without. TheZlibconstructor 4 more and 3 more. TheBrotliandZstdconstructors 0.*Synccall: 1 more bytecode instruction, the compare after the loop ofprocessChunkSync. Each async write: 0. The new code ofprocessCallbackis inside the branch for input that is left.inflateSync16.23,inflatewith a callback 48.21. For 2000 streams the counts for each cell type are identical.Each process:
.text: 108,801,193 to 108,801,449 bytes (+256). 163 of them are code in 4 functions: theinitthunk +143,do_work_inflate+13,NativeZlibClass__construct+4,NativeZlibPrototype__init+3. The other 93 are padding..bun_builtins, the sources of the built-in JS modules: 2,748,332 to 2,748,922 bytes (+590)..rodata,.dataand.bss: no change.node:zlib:ZlibBase173 to 184 instructions,Zlib147 to 152,processChunkSync180 to 192,processCallback155 to 173.Brotli,ZstdandzlibOnErrordo not change.size_oftheContextofNativeZlib: 48 before and after. A const assertion holds it.GeneratedNodeZlibBinding.cpp: 22 before and after. Oneinitthunk converts one more argument.Differences from Node, on purpose
processCallbackclearshandle.bufferon the error path. Node leaves the chunk there.zlib.tskeepsbufferset only while a write is on the threadpool.initargument that is not a boolean throwsERR_INVALID_ARG_TYPE. Node aborts the process.ZlibBasereads the option one time, then validates and stores that value. Node reads it 2 times, and 3 times when it is set. The lint ruleno-duplicate-conditional-property-accessof this repository asks for the one read.Not changed
NativeBrotli.inittakes no flag, as in Node. The JS check covers brotli.node:zlib/iterhas no such option in Node.DecompressionStreamis native and does not usenode:zlib. It keeps decoding a second gzip member (test/js/web/streams/compression.test.ts).packages/bun-types: the option types ofnode:zlibcome from@types/node. Version 26.6.5 declares the option.test-zlib-reject-garbage-after-end.js(v26.12.1) is not vendored, because a vendored file has to pass as a whole. 10 of its 12 tests pass with this PR, 3 of 12 on main. The other 2 need the zstd frame state, and one of the 2 asserts the zstd default.test-zlib-type-error.jsis not refreshed to v26.12.1. 3 of its 4 tests pass with this PR, 2 of 4 on main. Its gzipDecompressionStreamcase expects aTypeErrorfor a second gzip member, andtest/js/web/streams/compression.test.tsexpects Bun to decode it.The option on other paths
The check is "a write left input and had output space". Two more paths reach it, and Node.js v26.12.1 gives the same result for both:
inflateSync(valid, { finishFlush: Z_BLOCK, rejectGarbageAfterEnd: true })is the error.nbWorkerscan leave input in a write. With the option that is the error. Without the option the stream ends with no output, on main too.Found on the way, not in this PR
init()on aNativeZlibhandle leaks the first zlib stream. 5000 calls take main from 36 MiB to 1037 MiB. It has its own fix in progress.stream._handle.onerrorreads back as an internal object insideAsyncLocalStorage.run. It has its own fix in progress.initargument makes a new handle for each call, so it does not reach that leak.Test runs
bun bd test test/js/node/zlib/reject-garbage-after-end.test.ts: 124 pass, 6 todo, 0 fail. With--todothe 6 still fail. With the environment of the ASAN lane (leak check and exception check): exit 0.node --test): 116 pass, 6 todo, 0 fail. With eachtodoturned into a test: 122 pass.node:testandnode:assert, so Node runs the identical file and both results are above.zlib.test.jsimportsbun:test, so this is a new file.Self-review
The review raised 9 concerns.
initargument callediniton a handle that was already initialized (now a new handle for each call), a test title said that compressors ignore the option (the title and this body now say what the check is), the zstd cases that stay open were not listed or tested (nowtodotests, one live test and the table above), and the divergence forhandle.bufferhad no comment.reset()of the frame state without a test, an instruction count that was true only inside a frame, and the effect of the default change onminizlib.[human-review] gate passed · iteration 1 · 6 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file