Closing zlib stream may throw uncaught exception

[nakedible-p]

• Version: v6.11.3
• Platform: Linux bono 4.12.0-2-amd64 deps: update openssl to 1.0.1j #1 SMP Debian 4.12.13-1 (2017-09-19) x86_64 GNU/Linux
• Subsystem: zlib

When calling .close() on a zlib.createGunzip() stream while the engine is still working on decompressing some stuff, sometimes this exception is thrown:

zlib.js:633
      var newReq = self._handle.write(flushFlag,
                               ^

TypeError: Cannot read property 'write' of null
    at Zlib.callback (zlib.js:633:32)

Looking at the code, it seems that when _processChunk sets a callback, it does not sufficiently check that this._handle has not been unset in the meantime. The earlier code has assert(this._handle, 'zlib binding closed'); check, but the callback code does not.

But even the assert is wrong, as calling .close() on a stream, even when there's unprocessed data, should not throw any exceptions. I guess the function should check for this._closed similarily as it checks for this._hadError currently.

Unfortunately this bug is triggered very rarely on a production system, so I haven't been able to write short code to reproduce the issue.

[Fishrock123]

Sounds like it may be a good idea to attach an appropriate 'error' event listener on the stream if possible?

cc @nodejs/streams as there could still be an issue in here

[nakedible-p]

Adding an 'error' listener does nothing as the error is not emitted. Instead, it is thrown from callback that is invoked by the zlib binding, meaning that the only way to catch it is process.on('uncaughtException').

That said, even if the error listener would work, there is still a bug here - closing a stream should not emit an error either.

[mcollina]

I think this is fixed in 8: https://github.com/nodejs/node/blob/master/lib/zlib.js#L465-L484 vs https://github.com/nodejs/node/blob/v6.x/lib/zlib.js#L622-L643. The PR is #13322, but that's not easily backportable, so we might want to do an ad-hoc fix for Node 6.

[nakedible-p]

True, this seems fixed in 8. Would need testing to be sure, but unfortunately switching to 8 would be really difficult.

FWIW: We are currently testing stream._hadError = true; stream.close() as an ugly hack to fix this. Not sure of the results yet.

[nakedible-p]

I can definitely confirm that using stream._hadError = true; before closing the stream is a workaround that fixes the issue. Hopefully something similar could be implemented in to 6.x.

[mcollina]

@nakedible-p would you like to send a PR?

[MylesBorins]

@nakedible-p we are putting out an r.c. for the next 6.x today. If you can get a PR in within the next week we can get it in a future RC before the release

[nakedible-p]

I can do a fix PR for certain, but I have no idea how I would write a testcase for this. If a simple fix is fine, I'll get it done right away.

[mcollina]

@Trott #13322 fixed this. We need a separate fix for this one as well.

[nakedible-p]

@MylesBorins @mcollina There is now an extremely minimal fix PR. Let me know what else should I do.

[apapirovski]

Looks like this was resolved in #16312 — no clue why it didn't autoclose.