Skip to content

zlib: reject reset after gzip emitted incomplete output - #66179

Open
xia-chao wants to merge 2 commits into
nodejs:mainfrom
xia-chao:zlib-reject-reset-incomplete-output
Open

xia-chao wants to merge 2 commits into
nodejs:mainfrom
xia-chao:zlib-reject-reset-incomplete-output

Conversation

@xia-chao

@xia-chao xia-chao commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

gzip writes a header on the first write. Calling reset after that starts a new member while that header is already in the output, so gunzip fails.

Same idea as the zstd incomplete-frame case in #66088, but only for gzip. zlib deflate can still reset after a flush: the dictionary test throws the first output away and reuses the compressor. Raw deflate is unchanged.

reset before any write still works. flush then end, without reset, still works.

I ran the new gzip reset test, the dictionary test, the existing reset tests, test-zlib.js, and the flush tests on a Release build.

deflateReset starts a new member. Bytes already written out cannot be
taken back, so gunzip/inflate see a truncated member followed by a new
header. Refuse reset in that case. Raw deflate has no wrapper header
and is unchanged.

Signed-off-by: Xia Chao <shapirolutts@gmail.com>
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies. labels Sep 21, 2026
zlib-wrapped deflate still allows reset after flush. The dictionary
test discards the first member and reuses the compressor.

Signed-off-by: Xia Chao <shapirolutts@gmail.com>
@xia-chao xia-chao changed the title zlib: reject reset after gzip/deflate emitted incomplete output zlib: reject reset after gzip emitted incomplete output Sep 21, 2026
@codecov

codecov Bot commented Sep 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 88.88889% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.29%. Comparing base (513981d) to head (96593b0).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
src/node_zlib.cc 88.88% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #66179   +/-   ##
=======================================
  Coverage   90.28%   90.29%           
=======================================
  Files         790      790           
  Lines      272043   272062   +19     
  Branches    51945    51945           
=======================================
+ Hits       245619   245650   +31     
+ Misses      16931    16916   -15     
- Partials     9493     9496    +3     
Files with missing lines Coverage Δ
src/node_zlib.cc 80.00% <88.88%> (+0.20%) ⬆️

... and 27 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@xia-chao

xia-chao commented Sep 22, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review! Narrowed this to gzip only in a follow-up commit — could you take another look?

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

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. zlib Issues and PRs related to the zlib module and its compression dependencies.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants