GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — zlib: fix gzip member head/buffer boundary issue (backport of #5883) by addaleax · Pull Request #5973 · nodejs/node

Conversation

@addaleax

@addaleax addaleax commented Mar 31, 2016

Copy link
Copy Markdown
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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mscdex mscdex added zlib Issues and PRs related to the zlib module and its compression dependencies. v5.x labels Mar 31, 2016
@evanlucas
evanlucas force-pushed the v5.x branch from ec6b154 to d044898 Compare March 31, 2016 14:41
@Fishrock123

Fishrock123 commented Mar 31, 2016

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas

evanlucas commented Mar 31, 2016

Copy link
Copy Markdown
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

addaleax commented Mar 31, 2016

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@evanlucas
evanlucas force-pushed the v5.x branch from ae15d68 to c7dcffb Compare March 31, 2016 23:12
@bnoordhuis

bnoordhuis commented Apr 1, 2016

Copy link
Copy Markdown
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.)
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax
zlib: fix gzip member head/buffer boundary issue
172c991 
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

addaleax commented Apr 2, 2016

Copy link
Copy Markdown
Member Author

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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Apr 5, 2016

Copy link
Copy Markdown
Member

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/2165/
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 7, 2016

Copy link
Copy Markdown
Member

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell

jasnell commented Apr 7, 2016

Copy link
Copy Markdown
Member

LGTM if CI is green.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bnoordhuis

bnoordhuis commented Apr 14, 2016

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@addaleax

addaleax commented Apr 14, 2016

Copy link
Copy Markdown
Member Author

Yup, CI is green now 👍
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
jasnell pushed a commit that referenced this pull request Apr 15, 2016
@addaleax @jasnell
zlib: fix gzip member head/buffer boundary issue
61167c3 
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

jasnell commented Apr 15, 2016

Copy link
Copy Markdown
Member

Landed in 61167c3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jasnell jasnell closed this Apr 15, 2016
This was referenced Apr 20, 2016
V5.10.2 proposal #6311
Closed
V5.11.0 proposal #6321
Closed
V5.11.0 proposal #6322
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

zlib Issues and PRs related to the zlib module and its compression dependencies.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@addaleax @Fishrock123 @evanlucas @bnoordhuis @jasnell @mscdex
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.