tls: unconsume stream on destroy by addaleax · Pull Request #17478 · nodejs/node · GitHub

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

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.

Let's change it to early return.

@indutny addressed your comment & added a test

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

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?

done!

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

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

The test file LGTM if my assumptions are correct.

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 :)

LGTM

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>

Landed in f96a86c

Normally we don't backport to LTS until things have been in a current release (9.x) for at least two weeks. This hasn't gone into a 9.x release yet, so we'd be talking 6 weeks before this could land in v8.x.

If this is a serious bug we should consider including it in the v8.9.4 release candidate build going out today, so that it goes out with v8.9.4 in two weeks.

What would be helpful is an idea of:

How much pain this bug causes (if it's causing a hard crash repeatedly I'd assume a lot!)
How many people are going to hit this
How minor the fix is (how much risk is there in rushing it)

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.

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.

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.

+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>

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

@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?

@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 outstanding review comments, 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 outstanding review comments, 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

hybrist 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 outstanding review comments, and a CI started. label Dec 13, 2017

addaleax closed this Dec 13, 2017

addaleax deleted the tls-17475-maybefix branch December 13, 2017 05:47

addaleax removed the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label 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

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

               #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
                 sni_context_.Reset();
               #endif  // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB

tls: unconsume stream on destroy by addaleax · Pull Request #17478 · nodejs/node · GitHub

Uh oh!

Uh oh!

tls: unconsume stream on destroy - #17478

tls: unconsume stream on destroy#17478addaleax wants to merge 3 commits intonodejs:masternodejs/node:masterfrom addaleax:tls-17475-maybefixaddaleax/node:tls-17475-maybefixCopy head branch name to clipboard

Conversation

addaleax commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

indutny left a comment

Choose a reason for hiding this comment

Uh oh!

indutny Dec 6, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 7, 2017

Uh oh!

BridgeAR Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Dec 12, 2017

Uh oh!

addaleax commented Dec 12, 2017

Uh oh!

hybrist left a comment

Choose a reason for hiding this comment

Uh oh!

hybrist Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina Dec 12, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

addaleax commented Dec 13, 2017

Uh oh!

gibfahn commented Dec 20, 2017

Uh oh!

mcollina commented Dec 20, 2017

Uh oh!

qubyte commented Dec 20, 2017

Uh oh!

odinho commented Dec 20, 2017

Uh oh!

MylesBorins commented Dec 20, 2017

Uh oh!

MylesBorins commented Jan 22, 2018

Uh oh!

addaleax commented Jan 22, 2018

Uh oh!

MylesBorins commented Jan 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

tls: unconsume stream on destroy#17478
addaleax wants to merge 3 commits into
nodejs:masterfrom
addaleax:tls-17475-maybefix

addaleax commented Dec 6, 2017 •
edited
Loading