Skip to content

node:zlib: refuse reset() of a gzip member that is open and has given output - #47364

Open
robobun wants to merge 2 commits into
mainfrom
robobun/042207aa/zlib-reset-incomplete-frame
Open

robobun wants to merge 2 commits into
mainfrom
robobun/042207aa/zlib-reset-incomplete-frame

Conversation

@robobun

@robobun robobun commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

Fix

  • The zlib Context records that the open gzip member has given output. Context::reset returns Node's error before deflateReset. The existing onerror path destroys the stream.
  • reset() on a closed stream throws Node's ERR_INTERNAL_ASSERTION. It was a TypeError.
  • Verified: test/js/node/zlib/zlib-reset-incomplete-frame.test.ts (main fails 21 of 32, Node.js v26.12.1 passes all) and Node's test-zlib-reset-incomplete-output.js, unmodified. Also 63 vendored test-zlib*.js files.
  • Self-reviewed: 21 concerns raised, 17 addressed. The other 4 ask for a leak fix in init() (see Notes).

Background

  • The first deflate() call puts out the gzip header. After reset() the end of that member never follows.
  • Context is the codec state behind the native handle (_handle) of one stream. do_work runs once per native write.
  • Considered a check in ZlibBase.prototype.reset: it misses _handle.reset() and a sync _processChunk(), and adds work to every write completion.

Downsides

  • Write, flush, reset() per segment on one Gzip stream now destroys the stream. With no 'error' listener the process exits. Node.js before v26.11.0 accepts it.
  • A Gzip native write runs 11 or 12 more instructions in do_work (16 → 27 or 28). Other modes are unchanged.
  • Release binary: +461 B.
Notes

What each runtime does

"main" is da867c6. Bun 1.4.3 gives the same results as main. hello is written first, and world is written with end() if the stream is still open.

stream steps main this PR Node.js v26.12.1
createGzip() write, reset() no error, output fails with Z_DATA_ERROR 'error' ERR_ZLIB_INCOMPLETE_FRAME, destroyed the same
createGzip() write, flush, reset() no error, Z_DATA_ERROR ERR_ZLIB_INCOMPLETE_FRAME the same
createGzip() params(), reset() no error ERR_ZLIB_INCOMPLETE_FRAME the same
createGzip() write, flush(Z_FINISH), reset() helloworld helloworld helloworld
createDeflate() write, flush, reset() no error, Z_DATA_ERROR the same the same
createDeflateRaw() write, flush, reset() helloworld the same the same
createBrotliCompress() write, flush, reset() no error, output fails to decode the same the same
the 6 decompressors half of the input, reset(), all of the input decodes the same the same
Gzip _handle writeSync(flush), reset() returns onerror(message, -2, "ERR_ZLIB_INCOMPLETE_FRAME") the same
each of the 11 kinds destroy(), reset() TypeError (release build) ERR_INTERNAL_ASSERTION "zlib binding closed" the same
createZstdCompress() write, flush, reset() no error, ZSTD_error_corruption_detected the same as main ERR_ZLIB_INCOMPLETE_FRAME

Probe scripts, one line per row, compared with Node.js v26.12.1 on release builds:

  • 34 rows (each compressor with 5 step lists, the 6 decompressors, 3 native handles): main differs in 9, this PR in 4.
  • 28 rows of stream and handle life cycles: main differs in 15, this PR in 4.
  • 18 more rows: main differs in 8, this PR in 3.
  • Each row that still differs is a ZstdCompress row.

The error

  • Error, code ERR_ZLIB_INCOMPLETE_FRAME, errno -2 (Z_STREAM_ERROR), message "Cannot reset a gzip stream with an incomplete member; end the stream or start a new gzip compressor".
  • The native handle gives the three values to onerror, and zlibOnError in zlib.ts makes the Error. Node.js does the same, and the code is not in its lib/internal/errors.js. So there is no ErrorCode.ts entry.
  • reset() returns undefined. The stream is already destroyed when it returns, and 'error' follows on the next tick.
  • A refused reset() does not change the codec. The member can still be finished, and it decodes.

Sources: ZlibContext::ResetStream and nodejs/node#66179. The Node.js documentation of zlib.reset() states the refusal, and states that zlib-wrapped deflate and raw deflate stay resettable.

One flag where Node.js has two

Node.js keeps stream_complete_ and output_emitted_, and refuses if !stream_complete_ && output_emitted_. The code sets stream_complete_ only where it clears output_emitted_, and a deflate() call that fails puts out no bytes. So the two are never both true, and the test is the same as output_emitted_. A second flag could never change the result, and no test could cover it.

Only the GZIP arm of do_work writes the flag. Node.js writes its flags for Deflate and DeflateRaw too and reads them only for GZIP.

The gunzip loop over the members of one input calls reset_z_stream(), which is reset() without the check. So that loop runs the same instructions as on main.

ZstdCompress is not in this PR

Measurements

Release builds of main da867c6 and of this PR, linux x64, one build directory.

  • Instructions per native write inside Context::do_work (gdb single-step count, callees not counted):
    • Gzip: 16 → 28 (a write that gives output), 27 (a write that gives none), 27 (Z_FINISH). The count includes the new do_work_gzip.
    • Deflate and DeflateRaw: 16 → 16. Inflate: 3 → 3.
    • Gunzip: one member 41 → 41, two members 100 → 100.
  • Same instructions as main after addresses are removed (llvm-objdump): brotli do_work, zstd do_work, the three check_error, the three write, the three write_sync, brotli reset, zstd reset. 13 compared, 0 differ. do_work_inflate and set_dictionary differ only in the byte offset of mode (0x28 → 0x29).
  • User instructions per 1-byte writeSync, end to end (perf_event_open, slope of 1e6 and 2e6 calls, median of 7 interleaved runs): gzip 1,037 → 1,047, deflate 912 → 911, inflate 964 → 963, zstd 841 → 842, brotli 919 → 922. The runs of one build spread over 5 to 43 instructions.
  • Allocations per native write: +0. do_work_gzip calls only deflate. heapStats().objectCount over 1e6 writeSync calls does not grow on main or with this PR.
  • Struct sizes: zlib Context 48 → 48 B. The flag takes a padding byte. NativeZlib 152 → 152 B (the argument of mi_must_malloc in the constructor).
  • Code: do_work 154 → 159 B and do_work_gzip 66 B. CompressionStreamImpl::reset of NativeZlib 744 → 773 B. Release .text +256 B and .bun_builtins +205 B. data and bss are the same. No host function and no bindgen entry is new.
  • reset() itself: the JS function has 6 → 24 bytecodes, and 5 → 8 run for an open stream. The native part of an accepted gzip reset runs 58 → 61 instructions.

valgrind, perf and bloaty are not on this machine. The numbers are from gdb, llvm-objdump, nm -S, llvm-size and a perf_event_open counter.

Tests

  • test/js/node/zlib/zlib-reset-incomplete-frame.test.ts uses node:test and node:assert. The same file passes under Node.js v26.12.1 (node --test, 32 pass). Node.js v26.3.0 fails 10 of 32, because it resets in silence. A debug build and a release build of this PR: 32 pass. A release build of main: 21 fail, 11 pass.
  • test-zlib-reset-incomplete-output.js in test/js/node/test/parallel/ is the file of Node.js v26.12.1, byte for byte. main fails 2 of 7. This PR passes 7.
  • The snapshot of members-of-prototype-objects-for-any-receiver.test.ts changes for reset() on a receiver with no _handle: ERR_INTERNAL_ASSERTION for an object, a number or a string, where main throws a TypeError. Node.js v26.12.1 gives the same answer for each receiver.
  • Both files pass on the debug ASAN build with the environment of the ASAN lane (detect_leaks=1, BUN_DESTRUCT_VM_ON_EXIT=1, BUN_JSC_validateExceptionChecks=1).
  • Clause deletions, each in a build where it can be selected at run time: no set of the flag (12 tests fail), no clear at Z_STREAM_END (2), Z_STREAM_END handled as the other results (4), no check in reset() (12), the flag assigned on each call (2), the flag set with no comparison of avail_out (2). The throw in zlib.ts is covered by 11 tests that fail on main. The clear in init has no test (see the next section).
  • Also run on the debug build: 63 test-zlib*.js, 6 test-stream-iter-transform*.js and 3 test-webstreams-*compression*.js vendored files, zlib.test.js (473 pass), zlib-reset-race.test.ts, zlib-onerror-reentrancy.test.ts, zlib-handle-bounds-check.test.ts, bytesWritten.test.ts, deflate-streaming.test.ts, zlib-estimated-size-gc.test.ts, finished-call-memory.test.ts, zlib.kMaxLength.global.test.js, test/regression/brotli-reset-leak.test.ts, test/js/web/streams/compression.test.ts (73 pass) and test/js/bun/util/zstd.test.ts (99 pass).
  • Not run: Windows and macOS.

A second init() leaks on main

  • Context::init clears the flag, as Node's ZlibContext::Init clears its two. So init() then reset() is accepted, as in Node.js.
  • That line has no test. A second init() on a live zlib handle calls deflateInit2 over the first state and leaks it. This is on main. LeakSanitizer reports 350,336 bytes for one such call on the debug ASAN build, so a test of this path aborts the ASAN lane.
  • The stream classes call init() one time. Only code that drives _handle itself can reach a second one.
  • The fix changes init for all 7 zlib modes, and Node.js does a different thing there (it keeps the first state). It is not in this PR.

Rows that still differ from Node.js

  • ZstdCompress: see above.
  • params() on a destroyed Gzip stream, also after a refused reset(). Node.js throws an uncaught ERR_INTERNAL_ASSERTION from paramsAfterFlushCallback. Bun returns and does not call the callback. Hardening follow-up: fix regressions from #46021 and more crashes #46270 made that choice.
  • A closed native handle: _handle.close(), _handle.reset(). Bun returns for each closed handle (node_zlib_binding.rs:597). Node.js aborts for Gzip ("Unreachable code reached" in InitZlib). The stream API cannot reach this state: it drops the handle when it closes it.

The reset() test files of Node.js v26.12.1

file main this PR owner of the rest
test-zlib-dictionary.js pass pass
test-zlib-reset-before-write.js pass pass
test-zlib-reset-during-write.js pass pass
test-zlib-reset-incomplete-output.js 2 of 7 fail pass
test-zlib-zstd-reset-incomplete-frame.js 2 of 5 fail 2 of 5 fail #47376
test-zlib-zstd-reset.js 3 of 3 fail 3 of 3 fail #44261
test-zlib-brotli-reset.js 2 of 2 fail 2 of 2 fail #47073 changes brotli reset()

Other pull requests

git merge-tree of this branch with the heads of #47079, #47073, #47140, #47027, #33413 and #46891 reports no conflict. #44261 conflicts with main in two files that this PR does not touch.

… output

reset() on a Gzip stream dropped the deflate state of a member whose
header was already in the output. The next member followed that
fragment, and gunzip failed on the whole output with Z_DATA_ERROR.

Node.js v26.11.0 and later refuse that reset (nodejs/node#66179). The
native handle reports ERR_ZLIB_INCOMPLETE_FRAME through onerror, and
the stream is destroyed.

The zlib Context now records that the open gzip member has given
output, and Context::reset returns the same error before it calls
deflateReset. ZlibBase.prototype.reset throws ERR_INTERNAL_ASSERTION
for a closed stream, as Node.js does.
@robobun

robobun commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 9:39 AM PT - Oct 29th, 2026

✅ @robobun, your commit cb6b302fbd5b9e1c487411a6e41777e45018a31e passed in Build #135173! 🎉


🧪   To try this PR locally:

bunx bun-pr 47364

That installs a local version of the PR into your bun-47364 executable, so you can run:

bun-47364 --bun

@robobun

robobun commented Oct 29, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

How the problem reproduces, on main (da867c6) and on Bun 1.4.3:

import zlib from "node:zlib";
const z = zlib.createGzip(), out = [];
z.on("error", e => console.log("error", e.code));
z.on("data", d => out.push(d));
await new Promise(r => z.write("hello", r));
await new Promise(r => z.flush(r));
z.reset();
if (!z.destroyed) {
  z.end("world");
  await new Promise(r => z.on("end", r));
  console.log(zlib.gunzipSync(Buffer.concat(out)).toString());
}
  • main: reset() reports nothing, and gunzipSync then throws Z_DATA_ERROR.
  • Node.js v26.12.1 and this PR: error ERR_ZLIB_INCOMPLETE_FRAME.

The fix is this pull request, #47364. The zstd half is the draft #47376, on top of #47079.

CI on cb6b302 (build 135173) passes.

@coderabbitai

coderabbitai Bot commented Oct 29, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between da867c6 and ccdc690.

Files selected for processing: 4
  • src/js/node/zlib.ts
  • src/runtime/node/zlib/NativeZlib.rs
  • test/js/node/test/parallel/test-zlib-reset-incomplete-output.js
  • test/js/node/zlib/zlib-reset-incomplete-frame.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

Walkthrough

GZIP reset now rejects incomplete members that have emitted output. The runtime tracks GZIP output, and new tests cover reset behavior across compression and decompression stream types.

Changes

GZIP Reset Handling

Layer / File(s) Summary
Native output tracking and reset rules
src/runtime/node/zlib/NativeZlib.rs
The native context tracks output from GZIP members. Reset returns ERR_ZLIB_INCOMPLETE_FRAME when an incomplete GZIP member has emitted output. The UNZIP multi-member loop uses the underlying reset helper.
Stream API and regression coverage
src/js/node/zlib.ts, test/js/node/test/parallel/test-zlib-reset-incomplete-output.js, test/js/node/zlib/zlib-reset-incomplete-frame.test.ts
The JavaScript reset method throws ERR_INTERNAL_ASSERTION when the handle is absent. Tests cover GZIP reset outcomes, other stream types, native-handle behavior, and reset after destruction.

Suggested reviewers: jarred-sumner

Priority: Normal

Merge Risk: Minimal · up to ccdc6

The change rejects gzip resets that would discard incomplete output and adds regression coverage. No concrete merge-blocking issue remains; mergeable subject to normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title names the main change: gzip reset() is refused when a member is open and has produced output. It matches the raw summary, which describes Context tracking gzip output and reset() returning E…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description is complete and relevant. It explains the problem, implementation, scope, limitations, and verification results, including untested platforms. It does not use the template headings ver…
Full details: Title check

Explanation

The title names the main change: gzip reset() is refused when a member is open and has produced output. It matches the raw summary, which describes Context tracking gzip output and reset() returning ERR_ZLIB_INCOMPLETE_FRAME. It is specific, concise, and free of file lists or vague terms. It is 76 characters, which is slightly long but still readable.

Full details: Description check

Explanation

The description is complete and relevant. It explains the problem, implementation, scope, limitations, and verification results, including untested platforms. It does not use the template headings verbatim, but it provides equivalent information for both required sections.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked: the new ERR_ZLIB_INCOMPLETE_FRAME reaches JS through the existing emit_error → onerror → zlibOnError path (node_zlib_binding.rs reset, zlib.ts zlibOnError), so no ErrorCode.ts entry is needed and the stream is destroyed with the exact message/errno/code; output_emitted is only read by reset() after the write_in_progress guard and only written by do_work_gzip, so there is no cross-thread race; the gunzip multi-member loop and the UNZIP→GUNZIP transition never route through do_work_gzip, so decompressors are unaffected.

Extended reasoning...

The change adds a single output_emitted flag to the zlib Context, splits GZIP out of the deflate arm of do_work, makes Context::reset refuse with Node's error when a gzip member has emitted output, and replaces a debug-only $assert in ZlibBase.prototype.reset with an ERR_INTERNAL_ASSERTION throw; it touches no security-sensitive surface. The inline finding (double init() leaking z_stream state) is pre-existing but is now exercised directly by the new test, and a second verified finding was not posted, so approval is not appropriate; the note records the concrete concerns examined and ruled out.

Comment thread src/runtime/node/zlib/NativeZlib.rs
…ION, as in Node.js

Node.js v26.12.1 gives the same answer for each receiver of
zlib.Deflate.prototype.reset: ERR_INTERNAL_ASSERTION for an object, a
number or a string, and a TypeError for undefined and null.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants