FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix zlib pseudo multi member failure by addaleax · Pull Request #5863 · nodejs/node · GitHub

/ node Public

Fix zlib pseudo multi member failure - #5863

Closed
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:fix-zlib-pseudo-multi-member-failure
ConversationCommits (2)ChecksFiles changed
Closed

Fix zlib pseudo multi member failure#5863
addaleax wants to merge 2 commits into
nodejs:masterfrom
addaleax:fix-zlib-pseudo-multi-member-failure

Conversation

addaleax commented Mar 23, 2016

Copy link
Copy Markdown
Member

Pull Request check-list

  • 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

Fixes #5852, which was introduced in f380db2 (#5120).

Only treat the gzip magic bytes, when encountered within the file after reading a single block, as the start of a new member when the previous member has ended.

I’m really not sure on whether “pseudo-multimember-gzip” & co. are the right names for the test files, but I don’t think there’s a specific term for this very situation. 😄

I’m also not sure whether the ctx->strm_.avail_in >= GZIP_MIN_HEADER_SIZE check in node_zlib.cc doesn’t open the possibility of another edge case, namely that there is a new gzip member starting, but not enough bytes have been read into the internal buffer. But that is very likely not related to #5852, since I assume it would result in truncated output rather than an error.

bnoordhuis reviewed Mar 23, 2016
View reviewed changes
Comment thread src/node_zlib.cc Outdated

bnoordhuis Mar 23, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Tiny style nit: can you put last clause on a separate line?

bnoordhuis commented Mar 23, 2016

Copy link
Copy Markdown
Member

Would it be possible to shrink the test fixtures? We distribute the test suite and they add another 128 kB.

benjamingr reviewed Mar 23, 2016
View reviewed changes
Comment thread test/parallel/test-zlib-from-concatenated-gzip.js Outdated

benjamingr Mar 23, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If you're using path.join here it should be path.join(common.fixturesDir, 'pseudo-multimember-gzip'); rather than concat the strings.

addaleax Mar 23, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

… silly me 😄

addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 6375567 to 84be337 Compare March 23, 2016 12:07

addaleax commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

Updated with your suggestions. I’ll see what I can do about the file sizes – it’s just not that easy to get specific bytes at specific positions in gzipped files. 😄

addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 84be337 to 74d56e0 Compare March 23, 2016 12:41

addaleax commented Mar 23, 2016

Copy link
Copy Markdown
Member Author

Okay, was easier than I thought, both files are now just a few hundred bytes together.

mscdex added the zlib Issues and PRs related to the zlib subsystem. label Mar 23, 2016

bnoordhuis commented Mar 23, 2016

Copy link
Copy Markdown
Member

Nice, thanks. LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/2040/

Fishrock123 reviewed Mar 23, 2016
View reviewed changes
Comment thread test/parallel/test-zlib-from-concatenated-gzip.js Outdated

Fishrock123 Mar 23, 2016

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

deepStrictEqual might be better?

addaleax Mar 23, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sure, why not. Updated using assert.deepStrictEqual.

Fishrock123 commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

LGTM minus a nit, CI failure is unrelated.

addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 74d56e0 to 4e4c3c2 Compare March 23, 2016 14:25
addaleax added 2 commits March 23, 2016 15:26
test: add tests for reproducing nodejs#5852
2dd5945 
Add test files that reliably reproduce nodejs#5852. The gzipped file
in test/fixtures/pseudo-multimember-gzip.gz contains the gzip
magic bytes exactly at the position that node encounters after having
read a single block, leading it to believe that a new data
member is starting.
zlib: Fix handling of gzip magic bytes mid-file
1b2900f 
Only treat the gzip magic bytes, when encountered within the file
after reading a single block, as the start of a new member when
the previous member has ended.

Fixes: nodejs#5852
addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 4e4c3c2 to 1b2900f Compare March 23, 2016 14:26

cjihrig commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

LGTM

jasnell commented Mar 23, 2016

Copy link
Copy Markdown
Member

LGTM

kthelgason commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

Wow, that was obvious in retrospect. Thanks!

Fishrock123 pushed a commit that referenced this pull request Mar 23, 2016
zlib: Fix handling of gzip magic bytes mid-file
0b3936b 
Only treat the gzip magic bytes, when encountered within the file
after reading a single block, as the start of a new member when
the previous member has ended.

Add test files that reliably reproduce #5852. The gzipped file
in test/fixtures/pseudo-multimember-gzip.gz contains the gzip
magic bytes exactly at the position that node encounters after having
read a single block, leading it to believe that a new data
member is starting.

Fixes: #5852
PR-URL: #5863
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>

Fishrock123 commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

Thanks! Landed in 0b3936b :)

jasnell mentioned this pull request Mar 23, 2016
Closed

kthelgason commented Mar 23, 2016

Copy link
Copy Markdown
Contributor

@addaleax the other edge-case you mentioned, do you mean that another member is starting but less that 10 bytes remain in the input buffer? I think you're right that this might occur (although pretty unlikely). We only actually need to check if there are two bytes remaining, enough for the magic byte comparison.

Then again even with that change, the edge-case still exists, albeit even less likely. If f.x. only one byte remains in the input buffer, in which case the output would be truncated anyway.

addaleax deleted the fix-zlib-pseudo-multi-member-failure branch March 23, 2016 22:42

addaleax commented Mar 24, 2016

Copy link
Copy Markdown
Member Author

@kthelgason Yes, that’s what I’m talking about. I’ll try and see whether I can get a test case for that together, but I agree, it’s definitely not very likely to occur in the wild.
And good to know you’re on board with this change!

addaleax mentioned this pull request Mar 24, 2016
Closed
3 tasks
evanlucas pushed a commit that referenced this pull request Mar 30, 2016
zlib: Fix handling of gzip magic bytes mid-file
1d4c751 
Only treat the gzip magic bytes, when encountered within the file
after reading a single block, as the start of a new member when
the previous member has ended.

Add test files that reliably reproduce #5852. The gzipped file
in test/fixtures/pseudo-multimember-gzip.gz contains the gzip
magic bytes exactly at the position that node encounters after having
read a single block, leading it to believe that a new data
member is starting.

Fixes: #5852
PR-URL: #5863
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
evanlucas mentioned this pull request Mar 31, 2016
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

zlib Issues and PRs related to the zlib subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

zlib.createGunzip "unknown compression method"

8 participants

Back | FazBrowse Home | New Git URL