[v6.x fix] lib: fix zlib async callback after close #16312

nakedible-p · 2017-10-19T07:33:31Z

A minimal change that will fix #15625. There are no test cases for this, as I have not been able to find a way to trigger this without actually running a busy system in production. The fix is also minimal because I have been able to verify in production that this fixes the issue, by using code like stream._hadError = true; stream.close(). The fix is for 6.x only, as the code has been heavily rewritten in 8.x and does not contain this bug anymore.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

zlib

mcollina

LGTM as the change. However we might need to add a unit test, if someone can figure out how.

MylesBorins · 2017-10-19T19:29:40Z

/cc @nodejs/streams @addaleax (re zlib) @nodejs/lts

MylesBorins · 2017-10-19T20:00:06Z

CI: https://ci.nodejs.org/job/node-test-pull-request/10860/

addaleax · 2017-10-22T15:18:53Z

lib/zlib.js

@@ -594,7 +594,7 @@ Zlib.prototype._processChunk = function(chunk, flushFlag, cb) {
      this.callback = null;
    }

-    if (self._hadError)
+    if (self._hadError || !self._handle)


I think self._hadError implies !self._handle, so this might just be if (!self._handle)?

If you want it that way, I can change it. However, I prefer it this way because:

This is a backport, so the impetus is on minimal changes rather than best code for future as the code is already obsolete

This makes it explicit (and easier to review) that the change can not cause additional breakage by removing some handling that needs to be explicitly checked that all sites which set _hadError also clear _handle.

In case _hadError behavior is changed due to some other case, relying on _handle might not be guaranteed

But, I don't really know the project well enough to know if my arguments are valid or not. So, either way is fine for me, just let me know if it would be better with just if (!self._handle).

Move the core logic from `LineParser` should fail handling into the recoverable error check for the REPL default eval. Fixes: nodejs#15704 Backport-PR-URL: nodejs#15773 PR-URL: nodejs#6171 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

MylesBorins · 2017-11-14T19:07:56Z

@nakedible can you please rebase and respond to @addaleax's comment

`crypto.createCipher()` sets the fixed IV derived from password and it leads to a security risk of nonce reuse when counter mode is used. A warning is emitted when CTR, GCM or CCM is used in `crypto.createCipher()` to notify users to avoid nonce reuse. Backport-PR-URL: nodejs#16583 Fixes: nodejs#13801 PR-URL: nodejs#13821 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Tobias Nießen <[email protected]>

EVP_CIPHER_CTX_FLAG_WRAP_ALLOW flag needs to be set in using wrap mode ciphers. In `crypto.createCipher()`, AES key wrap mode does not use a default IV defined in RFC3394 but a generated IV with `EVP_BytesToKey()` to be consistent API behaviors with other ciphers. The built-in AES wrap mode in OpenSSL is not supported in FIPS mode as http://openssl.6102.n7.nabble.com/AES-Key-Wrap-in-FIPS-Mode-td50238.html so its tests in FIPS mode are skipped. Backport-PR-URL: nodejs#16584 Fixes: nodejs#15009 PR-URL: nodejs#15037 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>

This exception can logically never happen because of the key stretching that takes place first. Failure must therefore be a bug in Node.js and not in the executing script. Backport-PR-URL: nodejs#16585 PR-URL: nodejs#15183 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Closing a zlib stream may throw an uncaught exception afterwards if there was a pending callback still to be invoked. This adds a very minimal fix to the issue as all of this code has been rewritten in later versions. Fixes: nodejs#15625

nakedible-p · 2017-11-14T20:14:09Z

Rebased. Sorry, sort of dropped the ball on this, because I couldn't get tests for v6.x-staging to pass on my machine. Was something to do with a couple network tests not returning ENOTFOUND but some error.

MylesBorins · 2017-11-26T01:35:34Z

@addaleax is this ready to land now?

MylesBorins · 2017-12-19T18:44:03Z

ping @addaleax

MylesBorins · 2018-01-17T06:52:04Z

ping @addaleax

addaleax · 2018-01-17T18:34:07Z

@MylesBorins Yes, this is still ready :)

Closing a zlib stream may throw an uncaught exception afterwards if there was a pending callback still to be invoked. This adds a very minimal fix to the issue as all of this code has been rewritten in later versions. Fixes: #15625 PR-URL: #16312 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2018-01-19T16:35:43Z

landed in 5707f83

Closing a zlib stream may throw an uncaught exception afterwards if there was a pending callback still to be invoked. This adds a very minimal fix to the issue as all of this code has been rewritten in later versions. Fixes: #15625 PR-URL: #16312 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

MylesBorins · 2018-02-12T06:06:46Z

This unfortunately broke ws on osx and needed to be backed out of v6.x

We currently have ws marked as flaky on linux and skipping on windows, so this will need to be tested against an OSX box. More than happy to help run the tests if that is an issue.

To reproduce with this branch run

npm i -g citgm
citgm -v verbose ws

edit: test failure was a timeout"

MylesBorins · 2018-04-13T04:16:41Z

Closing as we never had resolution

nodejs-github-bot added v6.x zlib Issues and PRs related to the zlib subsystem. labels Oct 19, 2017

mcollina approved these changes Oct 19, 2017

View reviewed changes

MylesBorins self-assigned this Oct 19, 2017

addaleax approved these changes Oct 22, 2017

View reviewed changes

MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from 9219283 to b0fadbe Compare October 26, 2017 04:22

shigeki and others added 4 commits November 14, 2017 14:09

lib: fix zlib async callback after close

a390a32

Closing a zlib stream may throw an uncaught exception afterwards if there was a pending callback still to be invoked. This adds a very minimal fix to the issue as all of this code has been rewritten in later versions. Fixes: nodejs#15625

nakedible-p force-pushed the fix-zlib-close-for-v6.x branch from 7ce4aea to a390a32 Compare November 14, 2017 20:01

MylesBorins force-pushed the v6.x-staging branch from fa922b9 to 50c3dab Compare November 28, 2017 04:15

MylesBorins force-pushed the v6.x-staging branch from 88b6795 to 3c4bb3c Compare January 17, 2018 08:10

MylesBorins closed this Jan 19, 2018

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

MylesBorins reopened this Feb 12, 2018

MylesBorins force-pushed the v6.x-staging branch 3 times, most recently from 5bdb18e to 691cd5a Compare February 13, 2018 00:33

MylesBorins added the stalled Issues and PRs that are stalled. label Mar 20, 2018

MylesBorins force-pushed the v6.x-staging branch 2 times, most recently from 0a4c79b to 988cca8 Compare March 30, 2018 03:28

MylesBorins closed this Apr 13, 2018

apapirovski mentioned this pull request Apr 13, 2018

Closing zlib stream may throw uncaught exception #15625

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[v6.x fix] lib: fix zlib async callback after close #16312

[v6.x fix] lib: fix zlib async callback after close #16312

nakedible-p commented Oct 19, 2017 •

edited

Loading

mcollina left a comment

MylesBorins commented Oct 19, 2017

MylesBorins commented Oct 19, 2017

addaleax Oct 22, 2017 •

edited

Loading

nakedible-p Nov 14, 2017

MylesBorins commented Nov 14, 2017

nakedible-p commented Nov 14, 2017

MylesBorins commented Nov 26, 2017

MylesBorins commented Dec 19, 2017

MylesBorins commented Jan 17, 2018

addaleax commented Jan 17, 2018

MylesBorins commented Jan 19, 2018

MylesBorins commented Feb 12, 2018 •

edited

Loading

MylesBorins commented Apr 13, 2018

[v6.x fix] lib: fix zlib async callback after close #16312

[v6.x fix] lib: fix zlib async callback after close #16312

Conversation

nakedible-p commented Oct 19, 2017 • edited Loading

Checklist

Affected core subsystem(s)

mcollina left a comment

Choose a reason for hiding this comment

MylesBorins commented Oct 19, 2017

MylesBorins commented Oct 19, 2017

addaleax Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

nakedible-p Nov 14, 2017

Choose a reason for hiding this comment

MylesBorins commented Nov 14, 2017

nakedible-p commented Nov 14, 2017

MylesBorins commented Nov 26, 2017

MylesBorins commented Dec 19, 2017

MylesBorins commented Jan 17, 2018

addaleax commented Jan 17, 2018

MylesBorins commented Jan 19, 2018

MylesBorins commented Feb 12, 2018 • edited Loading

MylesBorins commented Apr 13, 2018

nakedible-p commented Oct 19, 2017 •

edited

Loading

addaleax Oct 22, 2017 •

edited

Loading

MylesBorins commented Feb 12, 2018 •

edited

Loading