zlib: Allow zlib.gunzip features with zlib.unzip #5884

addaleax · 2016-03-24T02:45:03Z

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

Detect whether a gzip file is being passed to unzip* by testing the first bytes for the gzip magic bytes, and setting the decompression mode to GUNZIP or INFLATE according to the result.

This enables gzip-only features like multi-member support to be used together with the unzip* autodetection support and thereby makes gunzip* and unzip* return identical results for gzip input again.

This is more or less a follow-up change to #5120.

bnoordhuis · 2016-03-24T09:07:24Z

LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/2051/

I asked you in the other PR to remove the defines but that was before I'd seen this PR.

addaleax · 2016-03-24T11:21:14Z

Sorry, even if CI passes, I’ll have to update this with a somewhat different approach and more tests.
On the JS side, currently the assumption is made that availInAfter > 0 implies availOutAfter === 0, but with the break; statement introduced here that would no longer be the case.

addaleax · 2016-03-24T11:46:28Z

Okay, added a new commit with tests for the case that even the magic bytes don’t come in a single chunk (I know it’s quite an unlikely edge case, but I guess even that should be supported). The new test will require the bugfix from #5883 to pass, though.

kthelgason · 2016-03-24T13:38:01Z

This looks like something that should obviously be supported. The only relevant bytes here are the first two bytes of the file, right? When would these be in separate chunks?

addaleax · 2016-03-24T13:43:13Z

Yes, these are the only inspected bytes. And honestly, I have no idea how they might get split into separate chunks. 😄 But it could, because we don’t have control about where the data comes from, and I think that means either not supporting this feature at all or creating an edge case in which it does not work.
And if you have a better idea on how to implement this, I’m all for it! I’m pretty sure there must be more beautiful solutions than the nested switch in node_zlib.cc.

kthelgason · 2016-03-24T14:04:09Z

I guess if someone set the chunksize to 1 when constructing a Gunzip object it could happen, whether that's a very clever thing to do is another matter! In any case I agree that we should try to support every case that can occur or none at all.

addaleax · 2016-03-24T14:09:49Z

> zlib.createGunzip({chunkSize:1})
Error: Invalid chunk size: 1

I guess you’re talking about something like receiving Z_STREAM_END from zlib exactly when the input buffer ends with a gzip member? I don’t think that’s actually a problem, Z_STREAM_END gets pretty much treated the same as Z_OK if I read the code correctly.

kthelgason · 2016-03-24T14:21:57Z

Actually, your example is exactly what I was thinking of, but I'm happy to see that it isn't possible. But obviously one can construct pathological cases where this would happen, like passing one byte at a time to unzip.write.

Anyway, I give this a big 👍, thanks for improving on this 😃

addaleax · 2016-03-24T14:34:07Z

I think that should be covered by the test cases here, but I guess you can feel free to create more of them.

And again, good to have you on board with this! 😄

bnoordhuis · 2016-03-31T11:44:26Z

src/node_zlib.cc

@@ -236,6 +239,40 @@ class ZCtx : public AsyncWrap {
        ctx->err_ = deflate(&ctx->strm_, ctx->flush_);
        break;
      case UNZIP:
+        next_expected_header_byte = ctx->strm_.next_in;
+
+        switch (ctx->gzip_id_bytes_read_) {


Quick question: would it be necessary to check that ctx->strm_.avail_in > 0 before trying to read the first byte?

You’re right.

In that vein, shouldn't there be a similar check for GZIP_HEADER_ID2?

Another question: what happens when the state machines sees GZIP_HEADER_ID1 followed by a byte that's not GZIP_HEADER_ID2? Is the first byte lost?

Sorry, you’re right – updated again. And no, no bytes are lost in that case; there is always the fall-through in the outer switch from case UNZIP: to case INFLATE:, so all input ends up being passed to inflate().

addaleax · 2016-03-31T12:04:33Z

Rebased and updated with your suggestion; The tests are passing now that #5883 is merged.

jasnell · 2016-04-01T16:38:17Z

LGTM if @bnoordhuis is happy.

jasnell · 2016-04-01T16:38:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/2121/

bnoordhuis · 2016-04-01T17:02:48Z

LGTM with the style nit that comments should be capitalized and punctuated.

Detect whether a gzip file is being passed to `unzip*` by testing the first bytes for the gzip magic bytes, and setting the decompression mode to `GUNZIP` or `INFLATE` according to the result. This enables gzip-only features like multi-member support to be used together with the `unzip*` autodetection support and thereby makes `gunzip*` and `unzip*` return identical results for gzip input again. Add a simple test for checking that features specific to `zlib.gunzip`, notably support for multiple members, also work when using `zlib.unzip`.

addaleax · 2016-04-02T01:03:27Z

Updated the comments. :)

bnoordhuis · 2016-04-05T18:04:06Z

Thanks Anna, landed in 2d7e316.

Detect whether a gzip file is being passed to `unzip*` by testing the first bytes for the gzip magic bytes, and setting the decompression mode to `GUNZIP` or `INFLATE` according to the result. This enables gzip-only features like multi-member support to be used together with the `unzip*` autodetection support and thereby makes `gunzip*` and `unzip*` return identical results for gzip input again. Add a simple test for checking that features specific to `zlib.gunzip`, notably support for multiple members, also work when using `zlib.unzip`. PR-URL: #5884 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fishrock123 added the zlib Issues and PRs related to the zlib subsystem. label Mar 24, 2016

addaleax changed the title ~~Allow zlib.gunzip features with zlib.unzip~~ zlib: Allow zlib.gunzip features with zlib.unzip Mar 24, 2016

jasnell added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 24, 2016

addaleax force-pushed the allow-gunzip-features-with-unzip branch from 07ed63a to fd10a48 Compare March 24, 2016 12:09

addaleax mentioned this pull request Mar 24, 2016

zlib: Fix gzip member header/input buffer boundary issue #5883

Closed

3 tasks

bnoordhuis reviewed Mar 31, 2016
View reviewed changes

addaleax force-pushed the allow-gunzip-features-with-unzip branch from fd10a48 to 2598a0f Compare March 31, 2016 12:04

addaleax force-pushed the allow-gunzip-features-with-unzip branch from 2598a0f to a388a5e Compare March 31, 2016 12:34

addaleax force-pushed the allow-gunzip-features-with-unzip branch from a388a5e to a8e1fb5 Compare April 2, 2016 01:03

bnoordhuis closed this Apr 5, 2016

addaleax deleted the allow-gunzip-features-with-unzip branch April 5, 2016 18:05

MylesBorins mentioned this pull request Apr 21, 2016

V5.11.0 proposal #6321

Closed

MylesBorins mentioned this pull request Apr 21, 2016

V5.11.0 proposal #6322

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: Allow zlib.gunzip features with zlib.unzip #5884

zlib: Allow zlib.gunzip features with zlib.unzip #5884

addaleax commented Mar 24, 2016

bnoordhuis commented Mar 24, 2016

addaleax commented Mar 24, 2016

addaleax commented Mar 24, 2016

kthelgason commented Mar 24, 2016

addaleax commented Mar 24, 2016

kthelgason commented Mar 24, 2016

addaleax commented Mar 24, 2016

kthelgason commented Mar 24, 2016

addaleax commented Mar 24, 2016

bnoordhuis Mar 31, 2016

addaleax Mar 31, 2016

bnoordhuis Mar 31, 2016

addaleax Mar 31, 2016

addaleax commented Mar 31, 2016

jasnell commented Apr 1, 2016

jasnell commented Apr 1, 2016

bnoordhuis commented Apr 1, 2016

addaleax commented Apr 2, 2016

bnoordhuis commented Apr 5, 2016

zlib: Allow zlib.gunzip features with zlib.unzip #5884

zlib: Allow zlib.gunzip features with zlib.unzip #5884

Conversation

addaleax commented Mar 24, 2016

Pull Request check-list

Affected core subsystem(s)

Description of change

bnoordhuis commented Mar 24, 2016

addaleax commented Mar 24, 2016

addaleax commented Mar 24, 2016

kthelgason commented Mar 24, 2016

addaleax commented Mar 24, 2016

kthelgason commented Mar 24, 2016

addaleax commented Mar 24, 2016

kthelgason commented Mar 24, 2016

addaleax commented Mar 24, 2016

bnoordhuis Mar 31, 2016

Choose a reason for hiding this comment

addaleax Mar 31, 2016

Choose a reason for hiding this comment

bnoordhuis Mar 31, 2016

Choose a reason for hiding this comment

addaleax Mar 31, 2016

Choose a reason for hiding this comment

addaleax commented Mar 31, 2016

jasnell commented Apr 1, 2016

jasnell commented Apr 1, 2016

bnoordhuis commented Apr 1, 2016

addaleax commented Apr 2, 2016

bnoordhuis commented Apr 5, 2016