This repository has been archived by the owner. It is now read-only.

zlib does not properly report the errors #3230

Closed
SaltwaterC opened this Issue May 7, 2012 · 3 comments

Comments

Projects
None yet
3 participants
@SaltwaterC

Expected to see some errors if the library is fed with plaintext data. No dice. I see instead empty buffers (for convenience methods) and straight end event (for the zlib.create* streams).

$ echo 'foo' > file.txt
$ gzip -c file.txt > file.txt.gz
$ node gunz.js file.txt
end of decompression
$ node gunz.js file.txt.gz
foo

end of decompression
$ node gunz2.js file.txt

$ node gunz2.js file.txt.gz
foo

The source js sources:

// gunz.js
var zlib = require('zlib');
var fs = require('fs');

var gunzip = zlib.createGunzip(); // same thing with zlib.createUnzip()
var rs = fs.createReadStream(process.argv[2]);
rs.pipe(gunzip);

gunzip.on('data', function (data) {
    console.log(data.toString());
});

gunzip.on('end', function () {
    console.log('end of decompression');
});

gunzip.on('error', function (err) {
    console.error(err);
});
// gunz2.js
var zlib = require('zlib');
var fs = require('fs');

zlib.gunzip(fs.readFileSync(process.argv[2]), function (err, res) {
    if (err) {
        console.error(err);
    } else {
        console.log(res.toString());
    }
});

Reproduced with node v0.6.17 under the latest Ubuntu and Windows 7. Presumably all the versions are affected. Under Ubuntu I reproduced it with 0.6.0, 0.6.5, 0.6.10, and 0.6.15 as well.

bnoordhuis added a commit to bnoordhuis/node that referenced this issue May 7, 2012

zlib: fix error reporting
This commit is a back-port of the changes on the master branch.

Fixes #3230.
@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis May 7, 2012

Member

This is fixed in the master branch, see #3052.

@isaacs: 1565682 is a back-port of the changes to v0.6. A minor infidelity is that it also back-ports setDictionary() (it wasn't possible to exclude that commit without introducing tons of conflicts) but worst case, we simply comment out the NODE_SET_METHOD call.

Member

bnoordhuis commented May 7, 2012

This is fixed in the master branch, see #3052.

@isaacs: 1565682 is a back-port of the changes to v0.6. A minor infidelity is that it also back-ports setDictionary() (it wasn't possible to exclude that commit without introducing tons of conflicts) but worst case, we simply comment out the NODE_SET_METHOD call.

@isaacs

This comment has been minimized.

Show comment Hide comment
@isaacs

isaacs May 8, 2012

@bnoordhuis Commenting out the method assignment is fine. The main thing is that the exposed API surface shouldn't change.

isaacs commented May 8, 2012

@bnoordhuis Commenting out the method assignment is fine. The main thing is that the exposed API surface shouldn't change.

@bnoordhuis bnoordhuis closed this in ee437c0 May 9, 2012

@bnoordhuis

This comment has been minimized.

Show comment Hide comment
@bnoordhuis

bnoordhuis May 9, 2012

Member

Landed sans dictionary support in ee437c0. Thanks for the bug report.

Member

bnoordhuis commented May 9, 2012

Landed sans dictionary support in ee437c0. Thanks for the bug report.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.