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

zlib deflate/inflate failure #30976

Closed
jeffrson opened this issue Dec 15, 2019 · 5 comments
Closed

zlib deflate/inflate failure #30976

jeffrson opened this issue Dec 15, 2019 · 5 comments
Labels
zlib Issues and PRs related to the zlib subsystem.

Comments

@jeffrson
Copy link

Hi,

I reported an issue against ws (websockets/ws#1669) which actually can be traced back to NodeJS (851a691678 - #26363), introduced from 11.10.1 to 11.11.0.

Essentially there is some deflate'd data that cannot be uncompressed successfully. I hope it's okay to point to related issue for code samples and further discussion. If not please tell - I'll extend this report.

@lpinca
Copy link
Member

lpinca commented Dec 15, 2019

Here is a test case:

const zlib = require('zlib');

const data = zlib.deflateRawSync('Welcome');

const chunks = [];
const inflate = zlib.createInflateRaw();

inflate.on('data', function(chunk) {
  chunks.push(chunk);
});

inflate.write(data);
inflate.write(Buffer.from([0x00, 0x00, 0xff, 0xff]));
inflate.flush(function() {
  const buf = Buffer.concat(chunks);
  console.log(buf.toString());
});

The problem is that inflate.flush() callback is not called. This used to work before 28db96f.

Decompression follows the RFC 7692 specifications.

It is possible to use the 'end' event, for example if

inflate.on('end', function() {
  const buf = Buffer.concat(chunks);
  console.log(buf.toString());
});

is added to the above example it will work as expected but is it intended?

@lpinca
Copy link
Member

lpinca commented Dec 15, 2019

cc: @addaleax

@lpinca lpinca added the zlib Issues and PRs related to the zlib subsystem. label Dec 15, 2019
@addaleax
Copy link
Member

This isn’t specific to flushes – in

const zlib = require('zlib');

const data = zlib.deflateRawSync('Welcome');

const inflate = zlib.createInflateRaw();

inflate.resume();
inflate.write(data, () => console.log('write 1 done'));
inflate.write(Buffer.from([0x00]), () => console.log('write 2 done'));
inflate.write(Buffer.from([0x00]), () => console.log('write 3 done'));

only the first two writes finish. I’ll try to figure out why that is.

@lpinca
Copy link
Member

lpinca commented Dec 19, 2019

I did not investigate but I think it's because the inflate stream is no longer readable after a block with the BFINAL bit set to 1 is received.

@addaleax
Copy link
Member

@lpinca Yeah, but other Transform streams seem to still allow writes to finish, even after .push(null). So I’ll look into that.

addaleax added a commit to addaleax/node that referenced this issue Dec 24, 2019
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: nodejs#30976
BridgeAR pushed a commit that referenced this issue Jan 3, 2020
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: #30976

PR-URL: #31082
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
targos pushed a commit that referenced this issue Jan 14, 2020
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: #30976

PR-URL: #31082
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
BethGriggs pushed a commit that referenced this issue Feb 6, 2020
Call the callback for writes that occur after the stream is closed.
This also requires changes to the code to not call `.destroy()`
on the stream in `.on('end')`, and to ignore chunks written
afterwards.

Previously, these writes would just queue up silently, as their
`_write()` callback would never have been called.

Fixes: #30976

PR-URL: #31082
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
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 a pull request may close this issue.

3 participants