GeezerVison

View reviewed changes

Comment thread

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

BridgeAR Dec 12, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax reacted with thumbs up emoji

All reactions

👍 1 reaction

addaleax Dec 12, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

done!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR commented Dec 12, 2017

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

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 commented Dec 12, 2017

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax force-pushed the tls-17475-maybefix branch from f3d9a70 to b7949b0 Compare

December 12, 2017 17:59

hybrist approved these changes

View reviewed changes

hybrist left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The test file LGTM if my assumptions are correct.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

hybrist Dec 12, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax Dec 12, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

hybrist reacted with thumbs up emoji

All reactions

👍 1 reaction

mcollina approved these changes

View reviewed changes

mcollina left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/tls_wrap.cc

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

mcollina Dec 12, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

apapirovski reacted with thumbs up emoji

All reactions

👍 1 reaction

addaleax Dec 13, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

jasnell approved these changes

View reviewed changes

addaleax mentioned this pull request

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

Closed

4 tasks


          [squash] reference test

a951088

addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

addaleax added a commit to addaleax/node that referenced this pull request


          tls: unconsume stream on destroy

f96a86c

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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

addaleax commented Dec 13, 2017

Copy link

Copy Markdown

Member Author

Landed in f96a86c

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax closed this

addaleax deleted the tls-17475-maybefix branch

December 13, 2017 05:47

addaleax removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label

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

gibfahn commented Dec 20, 2017

Copy link

Copy Markdown

Member

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gibfahn mentioned this pull request

Dec 20, 2017

Closed

mcollina commented Dec 20, 2017

Copy link

Copy Markdown

Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

qubyte commented Dec 20, 2017

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

odinho commented Dec 20, 2017

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins commented Dec 20, 2017

Copy link

Copy Markdown

Contributor

+1 on waiting until the next release

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins pushed a commit that referenced this pull request

Jan 8, 2018


          tls: unconsume stream on destroy

41702ef

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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

MylesBorins mentioned this pull request

Jan 10, 2018

v9.4.0 proposal #18069

Merged

MylesBorins pushed a commit that referenced this pull request


          tls: unconsume stream on destroy

30c6076

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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

MylesBorins pushed a commit that referenced this pull request


          tls: unconsume stream on destroy

36abbce

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 <[email protected]>
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>

MylesBorins commented Jan 22, 2018

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels

addaleax commented Jan 22, 2018

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax added the lts-watch-v4.x label