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

Fix zlib pseudo multi member failure #5863

Closed

Conversation

addaleax
Copy link
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.

@@ -258,7 +258,7 @@ class ZCtx : public AsyncWrap {
}
}
while (ctx->strm_.avail_in >= GZIP_MIN_HEADER_SIZE &&
ctx->mode_ == GUNZIP) {
ctx->mode_ == GUNZIP && ctx->err_ == Z_STREAM_END) {
Copy link
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?

@bnoordhuis
Copy link
Member

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

// files that have the "right" magic bytes for starting a new gzip member
// in the middle of themselves, even if they are part of a single
// regularly compressed member
const pmmFile = path.join(common.fixturesDir + '/pseudo-multimember-gzip');
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

… silly me 😄

@addaleax addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 6375567 to 84be337 Compare March 23, 2016 12:07
@addaleax
Copy link
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 addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 84be337 to 74d56e0 Compare March 23, 2016 12:41
@addaleax
Copy link
Member Author

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

@mscdex mscdex added the zlib Issues and PRs related to the zlib subsystem. label Mar 23, 2016
@bnoordhuis
Copy link
Member

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

})
.on('data', (data) => pmmResultBuffers.push(data))
.on('finish', common.mustCall(() => {
assert.deepEqual(Buffer.concat(pmmResultBuffers), pmmExpected,
Copy link
Contributor

Choose a reason for hiding this comment

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

deepStrictEqual might be better?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, why not. Updated using assert.deepStrictEqual.

@Fishrock123
Copy link
Contributor

LGTM minus a nit, CI failure is unrelated.

@addaleax addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 74d56e0 to 4e4c3c2 Compare March 23, 2016 14:25
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
@addaleax addaleax force-pushed the fix-zlib-pseudo-multi-member-failure branch from 4e4c3c2 to 1b2900f Compare March 23, 2016 14:26
@cjihrig
Copy link
Contributor

cjihrig commented Mar 23, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Mar 23, 2016

LGTM

@kthelgason
Copy link
Contributor

Wow, that was obvious in retrospect. Thanks!

Fishrock123 pushed a commit that referenced this pull request Mar 23, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
@Fishrock123
Copy link
Contributor

Thanks! Landed in 0b3936b :)

@kthelgason
Copy link
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 addaleax deleted the fix-zlib-pseudo-multi-member-failure branch March 23, 2016 22:42
@addaleax
Copy link
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!

evanlucas pushed a commit that referenced this pull request Mar 30, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
evanlucas pushed a commit that referenced this pull request Mar 31, 2016
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 <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Jeremiah Senkpiel <[email protected]>
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.

zlib.createGunzip "unknown compression method"
8 participants