tls: Closing parent socket also closes the tls sock #25642

dnakamura · 2015-07-07T14:21:03Z

Fixes #25365 . At the moment if you close the parent socket of a a TLS socket the TLS socket is not aware of this and will segfault when you attempt to perform and IO operation on it. Further, if you close the parent socket and then the TLS socket, the parent socket will emit the close event twice (and the TLS socket wont fire at all). If you change the order the results are reversed, ie. closing the TLS then the parent socket will result in the TLS socket firing the close event twice and the parent socket wont fire at all.

This patch moves toward linking the parent and tls sockets. Calling destroy() on either of the the sockets causes both sockets to be destroyed and the close event emitted for both of them

thefourtheye · 2015-07-07T21:54:04Z

lib/_tls_wrap.js

@@ -238,6 +238,7 @@ function TLSSocket(socket, options) {

  if (socket) {
    this._parent = socket;
+    socket._destroy = function(exception){self._destroy(exception)};


Why not simply this._destroy.bind(this);?

I'm newish to javascript and I didn't know that was a thing

I would do:

function(err) { self._destroy(err); }

Last time I check it - bind was kind of slow.

@indutny Oh, okay. Thanks for the info :-)

This one is still there ;)

This one wasn't fixed. Please make it span multiple lines.

mhdawson · 2015-07-08T22:12:11Z

@indutny could you review this as well since one of your recent commits ( dnakamura/node@bada87b#diff-04c3a6bcf355f2e05e7700c1b253d475) looks like it addressed a similar parent/child related issue.

indutny · 2015-07-08T23:28:30Z

lib/net.js

@@ -456,7 +456,8 @@ Socket.prototype._destroy = function(exception, cb) {
    if (cb) cb(exception);
    if (exception && !self._writableState.errorEmitted) {
      process.nextTick(function() {
-        self.emit('error', exception);
+        for(var s = self; s !== null; s = s._parent)


indutny · 2015-07-08T23:29:11Z

@dnakamura I wonder if we may consider backporting nodejs/node@9b6b05556 instead?

dnakamura · 2015-07-09T15:54:23Z

@Induty As far as I can tell, nodejs/node@9b6b055 does not fix the problem (still able to reproduce on latest io.js). Also I have to double check the code but I think it may already be backported

indutny · 2015-07-10T00:23:10Z

lib/net.js

    });
    this._handle.onread = noop;
-    this._handle = null;
+    for(var s = this; s !== null; s = s._parent){


for (...)

indutny · 2015-07-10T00:24:57Z

@dnakamura you are absolutely right. It just looked strikingly similar, so I thought it might need the backport. Looked through the changes and they are looking good to me except few nits.

dnakamura · 2015-07-20T17:39:02Z

@indutny , @thefourtheye took your changes into account, can you take another look.
(also, I added another commit, so that I wouldn't destroy your comments on the original commit. Will squash it when everybody agrees fix is good.)

thefourtheye · 2015-07-21T19:29:28Z

lib/net.js

+  for (var s = this; s !== null; s = s._parent){
+     timers.unenroll(s);
+     s._connecting = false;
+     s.readable = s.writable = false;


If we are writing like this, why not just

s._connecting = s.readable = s.writable = false;

then?

indutny · 2015-07-22T20:04:46Z

LGTM, except some style nits. May I ask you to fix them before I will land this PR?

(It is going to be fun to forward-port it to io.js. I should probably do it myself right after landing it in v0.12)

dnakamura · 2015-07-31T14:36:30Z

@indutny Fixed style nits, can you take a look?

indutny · 2015-08-05T19:11:56Z

lib/net.js

-
-  for (var s = this; s !== null; s = s._parent)
-    timers.unenroll(s);
+  for (var s = this; s !== null; s = s._parent){


Space between ) and {. Didn't make lint complain about it?

indutny · 2015-08-05T19:13:33Z

Few last nits, thanks!

dnakamura · 2015-08-10T14:03:49Z

@indutny ok that should be all of them

dnakamura · 2015-08-13T14:34:18Z

@indutny ok, hopefully now I've got it 😄

thefourtheye · 2015-08-13T16:32:42Z

lib/_tls_wrap.js

@@ -238,6 +239,9 @@ function TLSSocket(socket, options) {

  if (socket) {
    this._parent = socket;
+    socket._destroy = function(exception) {
+      self._destroy(exception)


Semicolon at the end?

dnakamura · 2015-08-17T16:07:37Z

@thefourtheye fixed.

indutny · 2015-08-17T17:28:11Z

Awesome LGTM. @thefourtheye may I have your LGTM as well? ;)

thefourtheye · 2015-08-17T17:50:03Z

@indutny Changes are straight forward. LGTMT :)

indutny · 2015-08-18T04:46:58Z

@mhdawson @thefourtheye where should I land this?

thefourtheye · 2015-08-18T05:03:58Z

@indutny I am not familiar with this repo's pr landing process but going by the tags on this PR I would say v0.12

jasnell · 2015-08-18T06:02:57Z

I can get it landed. FWIW, we use the node-accept-pull-request job in https://jenkins-iojs.nodesource.com/ to land in joyent/node. @orangemocha has all the necessary details on that.

thefourtheye · 2015-08-18T06:05:59Z

@jasnell Thanks for the info :-)

jasnell · 2015-08-18T06:06:51Z

@thefourtheye ... btw, I can't seem to find your full name anywhere for the sign off ;-)

thefourtheye · 2015-08-18T06:09:04Z

@jasnell Its in nodejs/node's collaborator's list. Sakthipriyan Vairamani :-) It is nice working with you :-)

jasnell · 2015-08-18T06:10:49Z

The pleasure is mine.
Ok, job kicked off... assuming the CI is green, the commit will land (https://jenkins-iojs.nodesource.com/job/node-accept-pull-request/36/)

jasnell · 2015-08-18T06:14:37Z

@dnakamura ... if you're seeing this problem on io.js master also, can I ask for you to go ahead and port this and open a new PR against master on nodejs/node if you haven't done so already? :-)

jasnell · 2015-08-18T06:39:35Z

Hmm.. @dnakamura getting some failures in CI. Not clear if they are related to the change. Same failure on both Windows platforms. https://jenkins-iojs.nodesource.com/job/node-test-commit-windows/nodes=win2008r2/181/

dnakamura · 2015-08-18T17:54:31Z

@jasnell: yup it does seem to be me (except test-dns), looking into the root cause now

dnakamura · 2015-08-18T19:01:59Z

So as it turns out the issue is because now if the socket is destroyed from an error, both sockets (tls, and parent) emit an error. Since nobody is listening for errors on the parent socket it causes an unhandled error event. My first instinct is to only change the code so it only fires the error event on the lowest socket (from a parent-child view, highest from a protocol stack view). However I'm wondering if it might make more sense to always fire the error event on the same object _destroy() was called on. Thoughts?

indutny · 2015-08-18T19:05:59Z

@dnakamura or we may call it a breaking change, land it in nodejs/node repo. And watch for the error events ourselves. Maybe this is better?

indutny · 2015-08-18T19:06:39Z

Or just not emit errors on parent socket in v0.12. Technically, emitting close should be enough to let it handle socket state change.

dnakamura · 2015-08-26T16:47:09Z

@jasnell Ok that should fix it now. I have a couple tests still failing here, but they also fail on official builds, so I dont think its my fault

jasnell · 2015-08-27T18:53:41Z

As long as it passes CI, LGTM. I'll run it through the checker now.

PR-URL: #25642 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

jasnell · 2015-08-27T19:12:12Z

Landed in 0363cf4

PR-URL: nodejs#25642 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

dnakamura force-pushed the tlsfix branch from c4dd56b to 585062a Compare July 7, 2015 14:39

thefourtheye reviewed Jul 7, 2015
View reviewed changes

indutny reviewed Jul 8, 2015
View reviewed changes

indutny reviewed Jul 10, 2015
View reviewed changes

lib/net.js

});

this._handle.onread = noop;

this._handle = null;

for(var s = this; s !== null; s = s._parent){

Copy link

Member

indutny Jul 10, 2015

Choose a reason for hiding this comment

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

for (...)

mhdawson self-assigned this Jul 17, 2015

mhdawson added S-confirmed-bug v0.12 P-2 labels Jul 17, 2015

mhdawson added this to the 0.12.8 milestone Jul 17, 2015

thefourtheye reviewed Jul 21, 2015
View reviewed changes

dnakamura force-pushed the tlsfix branch from 5f0c458 to 846d35f Compare July 24, 2015 13:48

indutny reviewed Aug 5, 2015
View reviewed changes

dnakamura force-pushed the tlsfix branch 2 times, most recently from 8884c5c to be30464 Compare August 7, 2015 13:38

dnakamura force-pushed the tlsfix branch from be30464 to d852a45 Compare August 13, 2015 14:33

thefourtheye reviewed Aug 13, 2015
View reviewed changes

dnakamura force-pushed the tlsfix branch from d852a45 to aa49618 Compare August 17, 2015 16:07

tls: Closing parent socket also closes the tls sock

21085b7

dnakamura force-pushed the tlsfix branch from aa49618 to 21085b7 Compare August 26, 2015 15:09

jasnell pushed a commit that referenced this pull request Aug 27, 2015

tls: Closing parent socket also closes the tls sock

0363cf4

PR-URL: #25642 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

jasnell closed this Aug 27, 2015

rvagg mentioned this pull request Sep 11, 2015

Release proposal: v0.12.8 nodejs/node#2806

Merged

jBarz pushed a commit to ibmruntimes/node that referenced this pull request Nov 4, 2016

tls: Closing parent socket also closes the tls sock

924c643

PR-URL: nodejs#25642 Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>

tls: Closing parent socket also closes the tls sock #25642

tls: Closing parent socket also closes the tls sock #25642

Conversation

dnakamura commented Jul 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented Jul 8, 2015

Choose a reason for hiding this comment

indutny commented Jul 8, 2015

dnakamura commented Jul 9, 2015

Choose a reason for hiding this comment

indutny commented Jul 10, 2015

dnakamura commented Jul 20, 2015

Choose a reason for hiding this comment

indutny commented Jul 22, 2015

dnakamura commented Jul 31, 2015

Choose a reason for hiding this comment

indutny commented Aug 5, 2015

dnakamura commented Aug 10, 2015

dnakamura commented Aug 13, 2015

Choose a reason for hiding this comment

dnakamura commented Aug 17, 2015

indutny commented Aug 17, 2015

thefourtheye commented Aug 17, 2015

indutny commented Aug 18, 2015

thefourtheye commented Aug 18, 2015

jasnell commented Aug 18, 2015

thefourtheye commented Aug 18, 2015

jasnell commented Aug 18, 2015

thefourtheye commented Aug 18, 2015

jasnell commented Aug 18, 2015

jasnell commented Aug 18, 2015

jasnell commented Aug 18, 2015

dnakamura commented Aug 18, 2015

dnakamura commented Aug 18, 2015

indutny commented Aug 18, 2015

indutny commented Aug 18, 2015

dnakamura commented Aug 26, 2015

jasnell commented Aug 27, 2015

jasnell commented Aug 27, 2015