zlib: support concatenated archives #5120

kthelgason · 2016-02-06T13:55:52Z

This PR adds support for gunzipping archives that consists of multiple concatenated members.
This has not been supported by node's implementation so far but is a part of the spec rfc1952.

The problem was raised in #4306, and there is more detailed discussion on this feature in the issue.

I also refactored some of lib/zlib.js but I will submit that in a separate PR later.

Fishrock123 · 2016-02-06T16:05:40Z

cc @indutny or @bnoordhuis

bnoordhuis · 2016-02-06T17:56:09Z

src/node_zlib.cc

@@ -254,11 +253,22 @@ class ZCtx : public AsyncWrap {
            ctx->err_ = Z_NEED_DICT;
          }
        }
+        if (ctx->strm_.avail_in >= 2 && ctx->mode_ == GUNZIP) {


Magic constants need an explaining comment and should preferably be true, well-named constants.

Yeah, I thought this was a little silly. It should probably check to make sure that avail_in is at least equal to the gzip header size. I'll fix this.

kthelgason · 2016-02-06T18:07:18Z

Thanks for the review @bnoordhuis. I've addressed some individual comments and will be updating this PR to fix all the issues mentioned.

bnoordhuis · 2016-02-06T18:12:38Z

Can you post a comment when you're done? GH doesn't send notifications for new or updated commits.

kthelgason · 2016-02-06T21:17:58Z

I've updated this in line with the comments from @bnoordhuis.

bnoordhuis · 2016-02-07T17:46:08Z

src/node_zlib.cc

+          // member in the same archive, or just trailing garbage.
+          // Check the header to find out.
+          if (*ctx->strm_.next_in != GZIP_HEADER_ID1 ||
+              *(ctx->strm_.next_in + 1) != GZIP_HEADER_ID2) {


It's a little more idiomatic to write it as ctx->strm_.next_in[1] != GZIP_HEADER_ID2. Suggestion: use indexed notation for the first byte too, for consistency; that's what we do elsewhere.

bnoordhuis · 2016-02-07T17:49:55Z

LGTM with a style nit. Perhaps it would be good to add a test for an archive that looks like an concatenated archive but isn't, i.e., with trailing junk that just happens to start with the right magic bytes.

CI: https://ci.nodejs.org/job/node-test-pull-request/1580/ (currently private)

kthelgason · 2016-02-07T19:26:42Z

I thought about including such a test, but it's pretty unclear to me what the expected behaviour is. Technically, trailing garbage in a gzipped file is forbidden by the spec. It seems however that in practice some files are padded with '\0', and I've accounted for this in my implementation. Currently, if a file happens to contain trailing garbage that happens to begin with the gzip header id bytes it will not decompress correctly and an error will be thrown.
I felt this was ok as the archive is technically invalid.

EDIT: I just checked if gzip(1) supports this and it throws an error in this situation. Seeing as it's cited by the RFC as a reference implementation I think we should adopt that behaviour as well.

kthelgason · 2016-02-07T19:28:38Z

Addressed the style nit and force pushed.

Fishrock123 · 2016-02-07T19:40:23Z

@kthelgason About the trailing \0 thing -- does this change the behavior of that from the current one?

kthelgason · 2016-02-07T19:42:41Z

@Fishrock123 no, both implementations simply ignore the junk bytes at the end.

Fishrock123 · 2016-02-07T19:43:36Z

@kthelgason Seems fine to me then; let's discuss the trailing bytes thing in a different issue if it is important.

thefourtheye · 2016-02-07T19:59:26Z

test/parallel/test-zlib-from-concatenated-gzip.js

+
+assert.equal(zlib.gunzipSync(data).toString(), 'abcdef');
+
+zlib.gunzip(data, (_, result) => {


Please don't ignore the error. You can do assert.ifError

do we need to wrap this kind of callback in common.mustCall to make sure the assertions are really executed?

That's why common.mustCall exists! :)

kthelgason · 2016-02-07T20:30:08Z

@thefourtheye @targos @Fishrock123 fixed.

Regarding the trailing garbage bytes, I think opening another issue to discuss that (if necessary) would be a good idea.

jasnell · 2016-02-07T21:22:50Z

LGTM

bnoordhuis · 2016-02-07T21:43:18Z

LGTM. Is there much to discuss w.r.t. trailing garbage? Emitting an error is the obvious thing to do, right?

kthelgason · 2016-02-07T21:45:54Z

@bnoordhuis to me it seems that way yes. Esp. given that gzip(1) handles it that way.

bnoordhuis · 2016-02-07T21:48:33Z

Exactly. I'd add the test case to this PR. Might as well land it with full coverage from the start.

kthelgason · 2016-03-14T22:13:13Z

Thanks for the comment @rvagg and for the review @bnoordhuis. I've squashed the commits as recommended, and CI looks good (apart from the ARM failure but as pointed out by @jbergstroem I can't see that it's related to this at all).

bnoordhuis · 2016-03-15T15:50:18Z

Thanks Kári, landed in f380db2.

According to the spec gzipped archives can contain more than one compressed member. Previously Node's gzip implementation would only unzip the first member and throw away the rest of the compressed data. Issue #4306 is an example of this occurring in daily use. Fixes: #4306 PR-URL: #5120 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

@AndreasMadsen

Notable changes: * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

According to the spec gzipped archives can contain more than one compressed member. Previously Node's gzip implementation would only unzip the first member and throw away the rest of the compressed data. Issue #4306 is an example of this occurring in daily use. Fixes: #4306 PR-URL: #5120 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

@AndreasMadsen

Notable changes: * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

@AndreasMadsen

Notable changes: * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

@AndreasMadsen

Notable changes: * **contextify**: Fixed a memory consumption issue related to heavy use of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh) #5392 * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231 - breakout events from v8 to offer better support for external debuggers * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

@AndreasMadsen

Notable changes: * **contextify**: Fixed a memory consumption issue related to heavy use of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh) #5392 * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231 - breakout events from v8 to offer better support for external debuggers * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

kthelgason force-pushed the zlib-concatenated-archives branch 2 times, most recently from 267b15e to 017e172 Compare February 6, 2016 15:26

Fishrock123 added zlib Issues and PRs related to the zlib subsystem. semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 6, 2016

bnoordhuis reviewed Feb 6, 2016
View reviewed changes

kthelgason force-pushed the zlib-concatenated-archives branch from 017e172 to 30be769 Compare February 6, 2016 21:17

bnoordhuis reviewed Feb 7, 2016
View reviewed changes

kthelgason force-pushed the zlib-concatenated-archives branch from 30be769 to 30f5b38 Compare February 7, 2016 19:27

thefourtheye reviewed Feb 7, 2016
View reviewed changes

kthelgason force-pushed the zlib-concatenated-archives branch from 30f5b38 to f7b67e4 Compare February 7, 2016 20:28

bnoordhuis closed this Mar 15, 2016

kthelgason deleted the zlib-concatenated-archives branch March 15, 2016 17:12

evanlucas mentioned this pull request Mar 15, 2016

v5.9.0 proposal #5702

Merged

jasnell mentioned this pull request Mar 22, 2016

zlib.createGunzip "unknown compression method" #5852

Closed

addaleax mentioned this pull request Mar 23, 2016

Fix zlib pseudo multi member failure #5863

Closed

3 tasks

tstanev 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

This was referenced Mar 24, 2016

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

Closed

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

Closed

addaleax mentioned this pull request Apr 8, 2016

ZLIB failing to inflate entire .gz file that contains an extra field in the header #6116

Closed

jacobischwartz mentioned this pull request Apr 25, 2017

Only opens first few ALB logs when using Node 4.6 amazon-archives/cloudwatch-logs-centralize-logs#4

Open

vmarkovtsev mentioned this pull request Aug 30, 2024

Cannot load a gzipped JSON trace with multiple blocks google/perfetto#872

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zlib: support concatenated archives #5120

zlib: support concatenated archives #5120

kthelgason commented Feb 6, 2016

Fishrock123 commented Feb 6, 2016

bnoordhuis Feb 6, 2016

kthelgason Feb 6, 2016

kthelgason commented Feb 6, 2016

bnoordhuis commented Feb 6, 2016

kthelgason commented Feb 6, 2016

bnoordhuis Feb 7, 2016

bnoordhuis commented Feb 7, 2016

kthelgason commented Feb 7, 2016

kthelgason commented Feb 7, 2016

Fishrock123 commented Feb 7, 2016

kthelgason commented Feb 7, 2016

Fishrock123 commented Feb 7, 2016

thefourtheye Feb 7, 2016

targos Feb 7, 2016

Fishrock123 Feb 7, 2016

kthelgason commented Feb 7, 2016

jasnell commented Feb 7, 2016

bnoordhuis commented Feb 7, 2016

kthelgason commented Feb 7, 2016

bnoordhuis commented Feb 7, 2016

kthelgason commented Mar 14, 2016

bnoordhuis commented Mar 15, 2016


		assert.equal(zlib.gunzipSync(data).toString(), 'abcdef');

		zlib.gunzip(data, (_, result) => {

zlib: support concatenated archives #5120

zlib: support concatenated archives #5120

Conversation

kthelgason commented Feb 6, 2016

Fishrock123 commented Feb 6, 2016

bnoordhuis Feb 6, 2016

Choose a reason for hiding this comment

kthelgason Feb 6, 2016

Choose a reason for hiding this comment

kthelgason commented Feb 6, 2016

bnoordhuis commented Feb 6, 2016

kthelgason commented Feb 6, 2016

bnoordhuis Feb 7, 2016

Choose a reason for hiding this comment

bnoordhuis commented Feb 7, 2016

kthelgason commented Feb 7, 2016

kthelgason commented Feb 7, 2016

Fishrock123 commented Feb 7, 2016

kthelgason commented Feb 7, 2016

Fishrock123 commented Feb 7, 2016

thefourtheye Feb 7, 2016

Choose a reason for hiding this comment

targos Feb 7, 2016

Choose a reason for hiding this comment

Fishrock123 Feb 7, 2016

Choose a reason for hiding this comment

kthelgason commented Feb 7, 2016

jasnell commented Feb 7, 2016

bnoordhuis commented Feb 7, 2016

kthelgason commented Feb 7, 2016

bnoordhuis commented Feb 7, 2016

kthelgason commented Mar 14, 2016

bnoordhuis commented Mar 15, 2016