tls: keep track of stream that is closed #11776

jBarz · 2017-03-09T20:44:21Z

TLSWrap object keeps a pointer reference to the underlying
TCPWrap object. This TCPWrap object could be closed and deleted
by the event-loop which leaves us with a dangling pointer.
So the TLSWrap object needs to track the "close" event on the
TCPWrap object.

Checklist

make -j4 test (UNIX)
commit message follows commit guidelines

Affected core subsystem(s)

ssl, tls
Fixes #8074.

mscdex · 2017-03-09T21:09:25Z

/cc @nodejs/crypto

indutny

Generally LGTM, but left one question/comment.

indutny · 2017-03-09T21:12:11Z

lib/_tls_wrap.js

@@ -374,6 +374,12 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
    res = null;
  });

+  if (wrap instanceof net.Socket) {
+    wrap.on('close', function() {


Should we do it for every kind of stream and not just net.Socket?

Yes you are right. Made the change.

addaleax · 2017-03-10T00:22:49Z

lib/_tls_wrap.js

@@ -374,6 +375,12 @@ TLSSocket.prototype._wrapHandle = function(wrap) {
    res = null;
  });

+  if (wrap instanceof Stream) {


Hmm… does this play nicely with readable-stream? Or, equivalently, does the socket instanceof Duplex check in TLSSocket()?

Would it make sense if the StreamBase destructor had a way of telling the TLSWrap that it is getting destroyed? That certainly adds a bit of complexity but would also seem more reliable/“obviously correct” to me… @indutny ?

Is it possible to avoid instanceof entirely? It's not the fastest thing...

On second thought, we don't need it :-). I have removed it.

mscdex · 2017-03-10T04:48:11Z

Can some sort of test be added for this?

TLSWrap object keeps a pointer reference to the underlying TCPWrap object. This TCPWrap object could be closed and deleted by the event-loop which leaves us with a dangling pointer. So the TLSWrap object needs to track the "close" event on the TCPWrap object.

jasnell · 2017-03-15T05:06:18Z

@mscdex Looks like there's a test now. PTAL

jasnell · 2017-03-16T22:52:25Z

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

mscdex · 2017-03-16T23:04:19Z

LGTM

TLSWrap object keeps a pointer reference to the underlying TCPWrap object. This TCPWrap object could be closed and deleted by the event-loop which leaves us with a dangling pointer. So the TLSWrap object needs to track the "close" event on the TCPWrap object. PR-URL: #11776 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

jasnell · 2017-03-17T00:20:03Z

Landed in 4cdb0e8

Trott · 2017-03-18T14:47:07Z

Looks like the new test may be unreliable on Raspberry Pi.

https://ci.nodejs.org/job/node-test-binary-arm/6752/RUN_SUBSET=3,label=pi1-raspbian-wheezy/console

not ok 168 parallel/test-tls-socket-close
  ---
  duration_ms: 2.557
  severity: fail
  stack: |-
    events.js:184
          throw er; // Unhandled 'error' event
          ^
    
    Error: read ECONNRESET
        at exports._errnoException (util.js:1029:11)
        at TLSWrap.onread (net.js:605:26)
  ...

Trott · 2017-03-19T06:21:11Z

The test supplied with this PR segfaults with Node v7.0.0 but passes on v7.1.0 and more recent. Yet this PR is only 9 days old, which would have meant that v7.7.x was out already.

Is it possible the bug was fixed and we landed this change anyway?

Or maybe I'm just doing something wrong?

Or maybe it needs to be tested on a specific platform and it's not the one I'm running?

Trott · 2017-03-19T06:45:17Z

Ah, it triggers the failure, but not reliably.

jasnell · 2017-03-20T16:38:54Z

Interesting... will queue this up to do a bit more digging later on today

TLSWrap object keeps a pointer reference to the underlying TCPWrap object. This TCPWrap object could be closed and deleted by the event-loop which leaves us with a dangling pointer. So the TLSWrap object needs to track the "close" event on the TCPWrap object. PR-URL: nodejs#11776 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

MylesBorins · 2017-04-17T22:52:45Z

I've cherry-picked to both v4 and v6. Plan to do a v4 maintenance release with this change and a couple others. I also landed #11947, please let me know if we need to land anything else

TLSWrap object keeps a pointer reference to the underlying TCPWrap object. This TCPWrap object could be closed and deleted by the event-loop which leaves us with a dangling pointer. So the TLSWrap object needs to track the "close" event on the TCPWrap object. PR-URL: #11776 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12499

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12497

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12499

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) #9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) #11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) #11947 * (Ben Noordhuis) #11898 * (jBarz) #11776 PR-URL: #12497

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12499 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12499 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497 Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs#11947 * (Ben Noordhuis) nodejs#11898 * (jBarz) nodejs#11776 PR-URL: nodejs#12499

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs#11947 * (Ben Noordhuis) nodejs#11898 * (jBarz) nodejs#11776 PR-URL: nodejs#12497

TLSWrap object keeps a pointer reference to the underlying TCPWrap object. This TCPWrap object could be closed and deleted by the event-loop which leaves us with a dangling pointer. So the TLSWrap object needs to track the "close" event on the TCPWrap object. PR-URL: nodejs/node#11776 Reviewed-By: Fedor Indutny <fedor.indutny@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>

Notable Changes: * module: - The module loading global fallback to the Node executable's directory now works correctly on Windows. (Richard Lau) nodejs/node#9283 * src: - fix base64 decoding in rare edgecase (Nikolai Vavilov) nodejs/node#11995 * tls: - fix rare segmentation faults when using TLS * (Trevor Norris) nodejs/node#11947 * (Ben Noordhuis) nodejs/node#11898 * (jBarz) nodejs/node#11776 PR-URL: nodejs/node#12497

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 Mar 9, 2017

indutny approved these changes Mar 9, 2017

View reviewed changes

jBarz force-pushed the issue8074 branch from 48245cd to d292672 Compare March 9, 2017 21:51

addaleax reviewed Mar 10, 2017

View reviewed changes

jBarz force-pushed the issue8074 branch from d292672 to 978e37b Compare March 10, 2017 01:20

jasnell approved these changes Mar 10, 2017

View reviewed changes

jBarz force-pushed the issue8074 branch from 978e37b to 170ec1c Compare March 10, 2017 19:46

jBarz force-pushed the issue8074 branch from 170ec1c to fd13478 Compare March 14, 2017 04:26

jasnell approved these changes Mar 15, 2017

View reviewed changes

addaleax mentioned this pull request Mar 16, 2017

segfault on node v6.3.1 on Ubuntu 14.04.4 #8074

Closed

jcstanaway mentioned this pull request Mar 16, 2017

SIGSEGV When Reading From SSL Stream #11885

Closed

jasnell closed this Mar 17, 2017

jBarz deleted the issue8074 branch March 17, 2017 02:47

jasnell self-assigned this Mar 20, 2017

italoacasas mentioned this pull request Mar 20, 2017

v7.7.4 Release Proposal #11941

Merged

addaleax mentioned this pull request Mar 20, 2017

async_hooks initial implementation #11883

Closed

4 tasks

addaleax added lts-watch-v4.x labels Mar 27, 2017

MylesBorins mentioned this pull request Apr 17, 2017

stream_base,tls_wrap: notify on destruct #11947

Closed

3 tasks

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Apr 17, 2017

This was referenced Apr 19, 2017

v6.10.3 proposal #12498

Merged

v4.8.3 proposal #12499

Merged

sam-github mentioned this pull request Aug 3, 2017

doc: describe what security issues are #14485

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tls: keep track of stream that is closed #11776

tls: keep track of stream that is closed #11776

jBarz commented Mar 9, 2017 •

edited

mscdex commented Mar 9, 2017

indutny left a comment

indutny Mar 9, 2017

jBarz Mar 9, 2017

addaleax Mar 10, 2017

addaleax Mar 10, 2017

mscdex Mar 10, 2017 •

edited

jBarz Mar 10, 2017

mscdex commented Mar 10, 2017

jasnell commented Mar 15, 2017

jasnell commented Mar 16, 2017

mscdex commented Mar 16, 2017

jasnell commented Mar 17, 2017

Trott commented Mar 18, 2017

Trott commented Mar 19, 2017

Trott commented Mar 19, 2017

jasnell commented Mar 20, 2017

MylesBorins commented Apr 17, 2017 •

edited

tls: keep track of stream that is closed #11776

tls: keep track of stream that is closed #11776

Conversation

jBarz commented Mar 9, 2017 • edited

Checklist

Affected core subsystem(s)

mscdex commented Mar 9, 2017

indutny left a comment

Choose a reason for hiding this comment

indutny Mar 9, 2017

Choose a reason for hiding this comment

jBarz Mar 9, 2017

Choose a reason for hiding this comment

addaleax Mar 10, 2017

Choose a reason for hiding this comment

addaleax Mar 10, 2017

Choose a reason for hiding this comment

mscdex Mar 10, 2017 • edited

Choose a reason for hiding this comment

jBarz Mar 10, 2017

Choose a reason for hiding this comment

mscdex commented Mar 10, 2017

jasnell commented Mar 15, 2017

jasnell commented Mar 16, 2017

mscdex commented Mar 16, 2017

jasnell commented Mar 17, 2017

Trott commented Mar 18, 2017

Trott commented Mar 19, 2017

Trott commented Mar 19, 2017

jasnell commented Mar 20, 2017

MylesBorins commented Apr 17, 2017 • edited

jBarz commented Mar 9, 2017 •

edited

mscdex Mar 10, 2017 •

edited

MylesBorins commented Apr 17, 2017 •

edited