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

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

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

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 ?

hybrist

The test file LGTM if my assumptions are correct.

hybrist · 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

 #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.

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

Landed in f96a86c

gibfahn · 2017-12-20T11:20:58Z

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)

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

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?

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

src/tls_wrap.cc Outdated

Copy link

Member

indutny Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's change it to early return.

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

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 pending requests for changes, 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 pending requests for changes, 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

Uh oh!

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

addaleax commented Dec 6, 2017 •

edited

Loading