Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zlib: fix gzip member head/buffer boundary issue (backport of #5883) #5973

Closed
wants to merge 1 commit into from

Conversation

addaleax
Copy link
Member

Pull Request check-list

Please make sure to review and check all of these items:

  • Does make -j8 test (UNIX) or vcbuild test nosign (Windows) pass with
    this change (including linting)?
  • Is the commit message formatted according to CONTRIBUTING.md?
  • If this change fixes a bug (or a performance problem), is a regression
    test (or a benchmark) included?

Affected core subsystem(s)

zlib

Description of change

This is squashed together from the corresponding commit in master and this commit to make sure behaviour stays the same as in previous 5.x releases.

Make sure that, even if an inflate() call only sees the first few bytes of a following gzip member, all members are decompressed and part of the full output.

Adds tests for the special case that the first inflate() call receives only the first few bytes of a second gzip member but not the whole header (or even just the magic bytes).

This is a backport of #5883 and contains additional changes to make sure that the behaviour on encountering trailing garbage remains the same (namely to silently discard it if one full member has already been decompressed).

@mscdex mscdex added zlib Issues and PRs related to the zlib subsystem. v5.x labels Mar 31, 2016
@Fishrock123
Copy link
Contributor

@addaleax Is it possible you have the wrong base here?

@evanlucas
Copy link
Contributor

@Fishrock123 nope, that is on me. I had to remove a commit from v5.x because it ended up depending on a semver-major change.

@addaleax addaleax force-pushed the backport-5883-to-5.x branch 2 times, most recently from d1059e3 to 0810475 Compare March 31, 2016 14:50
@addaleax
Copy link
Member Author

Okay, @evanlucas beat me to answering – rebased this either way. :)

@bnoordhuis
Copy link
Member

@addaleax Can you do another rebase and force-push? The diff is currently +6,516 −650 with 728 files changed.

The zlib changes LGTM at a glance except for a style issue (capitalization/punctuation of comments.)

Make sure that, even if an `inflate()` call only sees the first
few bytes of a following gzip member, all members are decompressed
and part of the full output.

Adds tests for the special case that the first `inflate()` call
receives only the first few bytes of a second gzip member but
not the whole header (or even just the magic bytes).

This is a backport of nodejs#5883 and contains additional changes to
make sure that the behaviour on encountering trailing garbage
remains the same (namely to silently discard it if one full member
has already been decompressed).

 #PR-URL: nodejs#5883
 #Reviewed-By: Ben Noordhuis <[email protected]>
 #Reviewed-By: James M Snell <[email protected]>
@addaleax addaleax force-pushed the backport-5883-to-5.x branch from 0810475 to 172c991 Compare April 2, 2016 01:08
@addaleax
Copy link
Member Author

addaleax commented Apr 2, 2016

Sure, that seems to have been another force-push to v5.x. Anyway, rebased + updated the comment in node_zlib.cc (Left the ones in the test files as they are in master).

@bnoordhuis
Copy link
Member

@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

New CI: https://ci.nodejs.org/job/node-test-pull-request/2206/
The previous one had a rather significant build bot failure on one of the rasb. pi's. So running again just to be safe.

@jasnell
Copy link
Member

jasnell commented Apr 7, 2016

LGTM if CI is green.

@bnoordhuis
Copy link
Member

New CI: https://ci.nodejs.org/job/node-test-pull-request/2265/ - looks like transient issues with the previous one.

@addaleax
Copy link
Member Author

Yup, CI is green now 👍

jasnell pushed a commit that referenced this pull request Apr 15, 2016
Make sure that, even if an `inflate()` call only sees the first
few bytes of a following gzip member, all members are decompressed
and part of the full output.

Adds tests for the special case that the first `inflate()` call
receives only the first few bytes of a second gzip member but
not the whole header (or even just the magic bytes).

This is a backport of #5883 and contains additional changes to
make sure that the behaviour on encountering trailing garbage
remains the same (namely to silently discard it if one full member
has already been decompressed).

PR-URL: #5973
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 15, 2016

Landed in 61167c3

@jasnell jasnell closed this Apr 15, 2016
This was referenced Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zlib Issues and PRs related to the zlib subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants