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

addaleax · 2016-03-23T10:55:21Z

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 · 2016-03-23T11:58:45Z

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

bnoordhuis · 2016-03-23T12:01:39Z

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

benjamingr · 2016-03-23T12:02:49Z

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

… silly me 😄

addaleax · 2016-03-23T12:10:24Z

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 · 2016-03-23T12:46:45Z

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

bnoordhuis · 2016-03-23T12:53:06Z

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

Fishrock123 · 2016-03-23T14:12:10Z

deepStrictEqual might be better?

Sure, why not. Updated using assert.deepStrictEqual.

Fishrock123 · 2016-03-23T14:12:54Z

LGTM minus a nit, CI failure is unrelated.

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.

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

cjihrig · 2016-03-23T14:58:24Z

LGTM

jasnell · 2016-03-23T15:34:55Z

LGTM

kthelgason · 2016-03-23T16:15:07Z

Wow, that was obvious in retrospect. Thanks!

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 · 2016-03-23T17:09:19Z

Thanks! Landed in 0b3936b :)

kthelgason · 2016-03-23T19:31:54Z

@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 · 2016-03-24T00:50:07Z

@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!

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>

bnoordhuis reviewed Mar 23, 2016
View reviewed changes

Comment threadsrc/node_zlib.cc Outdated
bnoordhuisMar 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.
Tiny style nit: can you put last clause on a separate line?

benjamingr reviewed Mar 23, 2016
View reviewed changes

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

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

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

Fishrock123 reviewed Mar 23, 2016
View reviewed changes

addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 74d56e0 to 4e4c3c2Compare March 23, 2016 14:25

addaleax added 2 commits March 23, 2016 15:26

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 1b2900fCompare March 23, 2016 14:26

jasnell added the dont-land-on-v4.x label Mar 23, 2016

Fishrock123 closed this Mar 23, 2016

jasnell mentioned this pull request Mar 23, 2016
Regression in gunzip in 5.9.0 -- GZ files that used to uncompress no longer do #5871
Closed

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

addaleax mentioned this pull request Mar 24, 2016
zlib: Fix gzip member header/input buffer boundary issue #5883
Closed
3 tasks

evanlucas mentioned this pull request Mar 31, 2016
v5.10.0 Release Proposal #5970
Closed

Uh oh!

Uh oh!

Conversation

addaleax commented Mar 23, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuisMar 23, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Mar 23, 2016

Uh oh!

benjamingrMar 23, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxMar 23, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax commented Mar 23, 2016

Uh oh!

addaleax commented Mar 23, 2016

Uh oh!

bnoordhuis commented Mar 23, 2016

Uh oh!

Fishrock123Mar 23, 2016

Choose a reason for hiding this comment

Uh oh!

addaleaxMar 23, 2016

Choose a reason for hiding this comment

Uh oh!

Fishrock123 commented Mar 23, 2016

Uh oh!

cjihrig commented Mar 23, 2016

Uh oh!

jasnell commented Mar 23, 2016

Uh oh!

kthelgason commented Mar 23, 2016

Uh oh!

Fishrock123 commented Mar 23, 2016

Uh oh!

kthelgason commented Mar 23, 2016

Uh oh!

addaleax commented Mar 24, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants