Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tls: keep track of stream that is closed #11776

Closed
wants to merge 1 commit into from
Closed

Conversation

jBarz
Copy link
Contributor

@jBarz jBarz commented Mar 9, 2017

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.

@nodejs-github-bot 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
@mscdex
Copy link
Contributor

mscdex commented Mar 9, 2017

/cc @nodejs/crypto

Copy link
Member

@indutny indutny left a comment

Choose a reason for hiding this comment

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

Generally LGTM, but left one question/comment.

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

if (wrap instanceof net.Socket) {
wrap.on('close', function() {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes you are right. Made the change.

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

if (wrap instanceof Stream) {
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor

@mscdex mscdex Mar 10, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@mscdex
Copy link
Contributor

mscdex commented Mar 10, 2017

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
Copy link
Member

jasnell commented Mar 15, 2017

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

@jasnell
Copy link
Member

jasnell commented Mar 16, 2017

@mscdex
Copy link
Contributor

mscdex commented Mar 16, 2017

LGTM

jasnell pushed a commit that referenced this pull request Mar 16, 2017
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
Copy link
Member

jasnell commented Mar 17, 2017

Landed in 4cdb0e8

@jasnell jasnell closed this Mar 17, 2017
@jBarz jBarz deleted the issue8074 branch March 17, 2017 02:47
@Trott
Copy link
Member

Trott commented Mar 18, 2017

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
Copy link
Member

Trott commented Mar 19, 2017

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
Copy link
Member

Trott commented Mar 19, 2017

Ah, it triggers the failure, but not reliably.

@jasnell
Copy link
Member

jasnell commented Mar 20, 2017

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

@jasnell jasnell self-assigned this Mar 20, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
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>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
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
Copy link
Contributor

MylesBorins commented Apr 17, 2017

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

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
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>
MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
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>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
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>
This was referenced Apr 19, 2017
MylesBorins added a commit that referenced this pull request May 2, 2017
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
MylesBorins added a commit that referenced this pull request May 2, 2017
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
MylesBorins added a commit that referenced this pull request May 2, 2017
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
MylesBorins added a commit that referenced this pull request May 2, 2017
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
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
imyller added a commit to imyller/meta-nodejs that referenced this pull request May 2, 2017
    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>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
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
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

segfault on node v6.3.1 on Ubuntu 14.04.4
9 participants