tls: unconsume stream on destroy #17478

addaleax · 2017-12-06T04:40:55Z

When the TLS stream is destroyed for whatever reason,
we should unset all callbacks on the underlying transport
stream.

Fixes ~~Refs (maybe Fixes)~~: #17475

@indutny Could you take a look at this?

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)

tls

indutny

I think it should be a safe change to land, however I'm a bit worried that there is no test for this yet.

LGTM, though.

indutny · 2017-12-06T05:21:41Z

src/tls_wrap.cc

@@ -101,6 +101,15 @@ TLSWrap::~TLSWrap() {
 #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
  sni_context_.Reset();
 #endif  // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
+  if (stream_ != nullptr) {


Let's change it to early return.

addaleax · 2017-12-07T21:57:58Z

@indutny addressed your comment & added a test

CI: https://ci.nodejs.org/job/node-test-commit/14651/

BridgeAR · 2017-12-12T15:02:51Z

test/parallel/test-tls-transport-destroy-after-own-gc.js

+if (!common.hasCrypto)
+  common.skip('missing crypto');
+
+const assert = require('assert');


This is actually a unused var. As far as I see it you only wanted to deactivate it for clientTLSHandle. So you could just use // eslint-disable-next-line no-unused-vars?

BridgeAR · 2017-12-12T15:04:28Z

I think it would be good to get another pair of eyes on this before landing due to the changed code. Therefore I am removing the ready label in the meanwhile.

When the TLS stream is destroyed for whatever reason, we should unset all callbacks on the underlying transport stream. Fixes: nodejs#17475

addaleax · 2017-12-12T17:37:52Z

@nodejs/collaborators Could somebody review (at least) the test here, since that has been kind of implicitly been requested by @BridgeAR ?

jkrems

The test file LGTM if my assumptions are correct.

jkrems · 2017-12-12T18:07:39Z

test/parallel/test-tls-transport-destroy-after-own-gc.js

+'use strict';
+
+// Regression test for https://github.com/nodejs/node/issues/17475
+// Unfortunately, this tests only "works" reliably when checked with valgrind or


Just for clarity: I assume "works" in this context means "doesn't raise memory leak errors"..? And properly running the test is just using the --valgrind option?

@jkrems Yeah – it complains about accessing released memory when run with valgrind. “Unfortunately”, most of the time that doesn’t lead to crashes…

Running the test file with --valgrind should make that reflected in the exit code, yes :)

mcollina

LGTM

mcollina · 2017-12-12T18:22:42Z

src/tls_wrap.cc

@@ -101,6 +101,15 @@ TLSWrap::~TLSWrap() {
 #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
  sni_context_.Reset();
 #endif  // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
+


I would add a comment before this block linking to the issue.

I’ve added a reference to the test and a quick explanation, the test contains a link to the issue.

When the TLS stream is destroyed for whatever reason, we should unset all callbacks on the underlying transport stream. PR-URL: nodejs#17478 Fixes: nodejs#17475 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2017-12-13T05:45:39Z

Landed in f96a86c

mcollina · 2017-12-20T11:36:49Z

I don't think this needs to be rushed. This problem has likely be in for a very long time, and we just heard about it recently.

qubyte · 2017-12-20T11:37:24Z

Hitting this bug should be pretty rare. I'm seeing it, but my use case is very similar to the code in #17475 (remarkably, I encountered it about a day later). It caused a whole lot of pain, but I'm happily using a recent build which includes the fixes, and will move over to the next release of 9 when it appears. The fix itself appears fairly low risk.

On balance, I would probably not do anything different to the usual process. The bug has been around for a long time, and I know of only two times it has been encountered.

odinho · 2017-12-20T13:09:21Z

I don't disagree with anything above. But I want to point out that we've also seen this bug a long time, but recently (as in the last 2-3 weeks) it's happening a lot more often. We had some trouble nailing it back when it was a sporadic crash (and also sporadic failure in our unittests).

Due to the nature of the bug, that could be due to a multitude of reasons. And this increase in frequency might be due to some of our code or change in our usage.

I wanted to propose it at least.

MylesBorins · 2017-12-20T20:41:58Z

+1 on waiting until the next release

When the TLS stream is destroyed for whatever reason, we should unset all callbacks on the underlying transport stream. PR-URL: #17478 Fixes: #17475 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins · 2018-01-22T08:48:57Z

I've landed this on both v6.x and v8.x staging. Please lmk if there are objections

addaleax · 2018-01-22T14:46:11Z

@MylesBorins Given the kind of issue that this fixes (rare, hard-to-debug segfault), I would even like to see it on v4.x. Would that be okay with you?

MylesBorins · 2018-01-22T17:55:13Z

@addaleax there were no plans for another 4.x but I'm open to considering if you think this is a big enough deal. Would you be willing to open an issue on release to discuss?

When the TLS stream is destroyed for whatever reason, we should unset all callbacks on the underlying transport stream. PR-URL: #17478 Fixes: #17475 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [#16413](#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [#16413](#16413) * upgrade libuv to 1.19.1 (cjihrig) [#18260](#18260) * re land npm 5.6.0 (Myles Borins) [#18625](#18625) * ICU 60 bump (Steven R. Loomis) [#16876](#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [#16130](#16130) * warn on invalid authentication tag length (Tobias Nießen) [#17566](#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [#18004](#18004) * use typed array stack as fast path (Anna Henningsen) [#17780](#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [#17273](#17273) * separate missing from default context (Andreas Madsen) [#17273](#17273) * rename initTriggerId (Andreas Madsen) [#17273](#17273) * deprecate undocumented API (Andreas Madsen) [#16972](#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [#16998](#16998) * add trace events to async_hooks (Andreas Madsen) [#15538](#15538) * set HTTPParser trigger to socket (Andreas Madsen) [#18003](#18003) * add provider types for net server (Andreas Madsen) [#17157](#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [#16495](#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [#17033](#17033) * module: * add builtinModules (Jon Moss) [#16386](#16386) * replace default paths in require.resolve() (cjihrig) [#17113](#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [#17109](#17109) * add process.ppid (cjihrig) [#16839](#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [#16267](#16267) * add rawPacket in err of `clientError` event (XadillaX) [#17672](#17672) * better support for IPv6 addresses (Mattias Holmlund) [#14772](#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [#17662](#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [#18463](#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [#17478](#17478) * process: * improve unhandled rejection message (Madara Uchiha) [#17158](#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [#12860](#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [#17196](#17196) PR-URL: #18336

Notable changes: * deps: * update V8 to 6.2.414.46 (Michaël Zasso) [nodejs#16413](nodejs#16413) * revert ABI breaking changes in V8 6.2 (Anna Henningsen) [nodejs#16413](nodejs#16413) * upgrade libuv to 1.19.1 (cjihrig) [nodejs#18260](nodejs#18260) * re land npm 5.6.0 (Myles Borins) [nodejs#18625](nodejs#18625) * ICU 60 bump (Steven R. Loomis) [nodejs#16876](nodejs#16876) * crypto: * Support both OpenSSL 1.1.0 and 1.0.2 (David Benjamin) [nodejs#16130](nodejs#16130) * warn on invalid authentication tag length (Tobias Nießen) [nodejs#17566](nodejs#17566) * async_hooks: * update defaultTriggerAsyncIdScope for perf (Anatoli Papirovski) [nodejs#18004](nodejs#18004) * use typed array stack as fast path (Anna Henningsen) [nodejs#17780](nodejs#17780) * use scope for defaultTriggerAsyncId (Andreas Madsen) [nodejs#17273](nodejs#17273) * separate missing from default context (Andreas Madsen) [nodejs#17273](nodejs#17273) * rename initTriggerId (Andreas Madsen) [nodejs#17273](nodejs#17273) * deprecate undocumented API (Andreas Madsen) [nodejs#16972](nodejs#16972) * add destroy event for gced AsyncResources (Sebastian Mayr) [nodejs#16998](nodejs#16998) * add trace events to async_hooks (Andreas Madsen) [nodejs#15538](nodejs#15538) * set HTTPParser trigger to socket (Andreas Madsen) [nodejs#18003](nodejs#18003) * add provider types for net server (Andreas Madsen) [nodejs#17157](nodejs#17157) * n-api: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * cli: * add --stack-trace-limit to NODE_OPTIONS (Anna Henningsen) [nodejs#16495](nodejs#16495) * console: * add support for console.debug (Benjamin Zaslavsky) [nodejs#17033](nodejs#17033) * module: * add builtinModules (Jon Moss) [nodejs#16386](nodejs#16386) * replace default paths in require.resolve() (cjihrig) [nodejs#17113](nodejs#17113) * src: * add helper for addons to get the event loop (Anna Henningsen) [nodejs#17109](nodejs#17109) * add process.ppid (cjihrig) [nodejs#16839](nodejs#16839) * http: * support generic `Duplex` streams (Anna Henningsen) [nodejs#16267](nodejs#16267) * add rawPacket in err of `clientError` event (XadillaX) [nodejs#17672](nodejs#17672) * better support for IPv6 addresses (Mattias Holmlund) [nodejs#14772](nodejs#14772) * net: * remove ADDRCONFIG DNS hint on Windows (Bartosz Sosnowski) [nodejs#17662](nodejs#17662) * process: * fix reading zero-length env vars on win32 (Anna Henningsen) [nodejs#18463](nodejs#18463) * tls: * unconsume stream on destroy (Anna Henningsen) [nodejs#17478](nodejs#17478) * process: * improve unhandled rejection message (Madara Uchiha) [nodejs#17158](nodejs#17158) * stream: * remove usage of *State.highWaterMark (Calvin Metcalf) [nodejs#12860](nodejs#12860) * trace_events: * add executionAsyncId to init events (Andreas Madsen) [nodejs#17196](nodejs#17196) PR-URL: nodejs#18336

addaleax added the wip Issues and PRs that are still a work in progress. label Dec 6, 2017

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem. labels Dec 6, 2017

indutny approved these changes Dec 6, 2017

View reviewed changes

addaleax force-pushed the tls-17475-maybefix branch from 31f824d to 7f684a1 Compare December 7, 2017 20:35

addaleax removed the wip Issues and PRs that are still a work in progress. label Dec 7, 2017

addaleax force-pushed the tls-17475-maybefix branch from 7f684a1 to f3d9a70 Compare December 7, 2017 21:49

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 7, 2017

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

addaleax added lts-watch-v6.x labels Dec 9, 2017

addaleax mentioned this pull request Dec 10, 2017

Crash during Garbage collection in TLSWrap #17097

Closed

BridgeAR reviewed Dec 12, 2017

View reviewed changes

BridgeAR removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 12, 2017

addaleax added 2 commits December 12, 2017 18:33

tls: unconsume stream on destroy

e95cc8a

When the TLS stream is destroyed for whatever reason, we should unset all callbacks on the underlying transport stream. Fixes: nodejs#17475

[squash] bridgear comment

b7949b0

addaleax force-pushed the tls-17475-maybefix branch from f3d9a70 to b7949b0 Compare December 12, 2017 17:59

jkrems approved these changes Dec 12, 2017

View reviewed changes

mcollina approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

addaleax mentioned this pull request Dec 13, 2017

http2: refactor close/destroy for Http2Stream and Http2Session #17406

Closed

4 tasks

[squash] reference test

a951088

addaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 13, 2017

addaleax closed this Dec 13, 2017

gibfahn mentioned this pull request Dec 20, 2017

Heavy use of TLSSocket + tls.connect crashes with SIGSEGV/SIGABRT #17475

Closed

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Jan 22, 2018

addaleax added the lts-watch-v4.x label Jan 22, 2018

MylesBorins mentioned this pull request Jan 24, 2018

v6.13.0 proposal #18342

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: unconsume stream on destroy #17478

tls: unconsume stream on destroy #17478

addaleax commented Dec 6, 2017 •

edited

Loading

indutny left a comment

indutny Dec 6, 2017

addaleax commented Dec 7, 2017

BridgeAR Dec 12, 2017

addaleax Dec 12, 2017

BridgeAR commented Dec 12, 2017

addaleax commented Dec 12, 2017

jkrems left a comment

jkrems Dec 12, 2017

addaleax Dec 12, 2017

mcollina left a comment

mcollina Dec 12, 2017

addaleax Dec 13, 2017

addaleax commented Dec 13, 2017

mcollina commented Dec 20, 2017

qubyte commented Dec 20, 2017

odinho commented Dec 20, 2017

MylesBorins commented Dec 20, 2017

MylesBorins commented Jan 22, 2018

addaleax commented Jan 22, 2018

MylesBorins commented Jan 22, 2018

tls: unconsume stream on destroy #17478

tls: unconsume stream on destroy #17478

Conversation

addaleax commented Dec 6, 2017 • edited Loading

Checklist

Affected core subsystem(s)

indutny left a comment

Choose a reason for hiding this comment

indutny Dec 6, 2017

Choose a reason for hiding this comment

addaleax commented Dec 7, 2017

BridgeAR Dec 12, 2017

Choose a reason for hiding this comment

addaleax Dec 12, 2017

Choose a reason for hiding this comment

BridgeAR commented Dec 12, 2017

addaleax commented Dec 12, 2017

jkrems left a comment

Choose a reason for hiding this comment

jkrems Dec 12, 2017

Choose a reason for hiding this comment

addaleax Dec 12, 2017

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

mcollina Dec 12, 2017

Choose a reason for hiding this comment

addaleax Dec 13, 2017

Choose a reason for hiding this comment

addaleax commented Dec 13, 2017

mcollina commented Dec 20, 2017

qubyte commented Dec 20, 2017

odinho commented Dec 20, 2017

MylesBorins commented Dec 20, 2017

MylesBorins commented Jan 22, 2018

addaleax commented Jan 22, 2018

MylesBorins commented Jan 22, 2018

addaleax commented Dec 6, 2017 •

edited

Loading