Skip to content

Commit

Permalink
zlib: fix gzip member head/buffer boundary issue
Browse files Browse the repository at this point in the history
Make sure that, even if an `inflate()` call only sees the first
few bytes of a following gzip member, all members are decompressed
and part of the full output.

This change also modifies behaviour for trailing garbage:
If there is trailing garbage which happens to start with the
gzip magic bytes, it is no longer discarded but rather throws
an error, since we cannot reliably tell random garbage from
a valid gzip member anyway and have to try and decompress it.
(Null byte padding is not affected, since it has been pointed
out at various occasions that such padding is normal and
discarded by `gzip(1)`, too.)

Adds tests for the special case that the first `inflate()` call
receives only the first few bytes of a second gzip member but
not the whole header (or even just the magic bytes).

PR-URL: #5883
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
addaleax authored and bnoordhuis committed Mar 31, 2016
1 parent 33c27f8 commit 54a5287
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 14 deletions.
16 changes: 7 additions & 9 deletions src/node_zlib.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ enum node_zlib_mode {

#define GZIP_HEADER_ID1 0x1f
#define GZIP_HEADER_ID2 0x8b
#define GZIP_MIN_HEADER_SIZE 10

void InitZlib(v8::Local<v8::Object> target);

Expand Down Expand Up @@ -257,17 +256,16 @@ class ZCtx : public AsyncWrap {
ctx->err_ = Z_NEED_DICT;
}
}
while (ctx->strm_.avail_in >= GZIP_MIN_HEADER_SIZE &&

while (ctx->strm_.avail_in > 0 &&
ctx->mode_ == GUNZIP &&
ctx->err_ == Z_STREAM_END) {
ctx->err_ == Z_STREAM_END &&
ctx->strm_.next_in[0] != 0x00) {
// Bytes remain in input buffer. Perhaps this is another compressed
// member in the same archive, or just trailing garbage.
// Check the header to find out.
if (ctx->strm_.next_in[0] != GZIP_HEADER_ID1 ||
ctx->strm_.next_in[1] != GZIP_HEADER_ID2) {
// Not a valid gzip member
break;
}
// Trailing zero bytes are okay, though, since they are frequently
// used for padding.

Reset(ctx);
ctx->err_ = inflate(&ctx->strm_, ctx->flush_);
}
Expand Down
30 changes: 28 additions & 2 deletions test/parallel/test-zlib-from-concatenated-gzip.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,12 @@ const zlib = require('zlib');
const path = require('path');
const fs = require('fs');

const abcEncoded = zlib.gzipSync('abc');
const defEncoded = zlib.gzipSync('def');

const data = Buffer.concat([
zlib.gzipSync('abc'),
zlib.gzipSync('def')
abcEncoded,
defEncoded
]);

assert.equal(zlib.gunzipSync(data).toString(), 'abcdef');
Expand Down Expand Up @@ -38,3 +41,26 @@ fs.createReadStream(pmmFileGz)
assert.deepStrictEqual(Buffer.concat(pmmResultBuffers), pmmExpected,
'result should match original random garbage');
}));

// test that the next gzip member can wrap around the input buffer boundary
[0, 1, 2, 3, 4, defEncoded.length].forEach((offset) => {
const resultBuffers = [];

const unzip = zlib.createGunzip()
.on('error', (err) => {
assert.ifError(err);
})
.on('data', (data) => resultBuffers.push(data))
.on('finish', common.mustCall(() => {
assert.strictEqual(Buffer.concat(resultBuffers).toString(), 'abcdef',
`result should match original input (offset = ${offset})`);
}));

// first write: write "abc" + the first bytes of "def"
unzip.write(Buffer.concat([
abcEncoded, defEncoded.slice(0, offset)
]));

// write remaining bytes of "def"
unzip.end(defEncoded.slice(offset));
});
5 changes: 2 additions & 3 deletions test/parallel/test-zlib-from-gzip-with-trailing-garbage.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,8 @@ data = Buffer.concat([
Buffer([0x1f, 0x8b, 0xff, 0xff])
]);

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

zlib.gunzip(data, common.mustCall((err, result) => {
assert.ifError(err);
assert.equal(result, 'abcdef', 'result should match original string');
assert(err);
}));

0 comments on commit 54a5287

Please sign in to comment.