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: nullify .ssl on handle close #5168

Closed
wants to merge 1 commit into from
Closed

Conversation

indutny
Copy link
Member

@indutny indutny commented Feb 9, 2016

This is an intermediate fix for an issue of accessing TLSWrap fields
after the parent handle was destroyed. While close listener cleans up
this field automatically, it can be done even earlier at the
TLSWrap.close call.

Proper fix is going to be submitted and landed after this one.

Fix: #5108

R = @bnoordhuis or @shigeki

cc @nodejs/crypto

@indutny
Copy link
Member Author

indutny commented Feb 9, 2016

@indutny
Copy link
Member Author

indutny commented Feb 9, 2016

@bnoordhuis @shigeki I think proper fix will be to avoid calling ~HandleWrap until the TLSWrap will die. Do you have any opinion on this?

@ChALkeR
Copy link
Member

ChALkeR commented Feb 9, 2016

@indutny I will test this soon, sorry for the delay.

@indutny
Copy link
Member Author

indutny commented Feb 9, 2016

@ChALkeR no problem at all. Thanks for looking!

@indutny
Copy link
Member Author

indutny commented Feb 9, 2016

Gosh, forgot to commit test. Happens every other time!

This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.

Proper fix is going to be submitted and landed after this one.

Fix: nodejs#5108
@indutny
Copy link
Member Author

indutny commented Feb 9, 2016

@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Feb 9, 2016
@indutny indutny mentioned this pull request Feb 9, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2016

The fix works for me in both the small and the original testcase, thanks!

@shigeki
Copy link
Contributor

shigeki commented Feb 10, 2016

@indutny I still cannot figure out what causes this error. The test case seems to be equivalent to the followings and it also leads seg faults.

const server = tls.createServer(opts, function(c) {
  c.end();
}).listen(common.PORT, function() {
  var client = tls.connect({
    port: common.PORT,
    rejectUnauthorized: false
  }, function() {
    client.removeAllListeners('close');
    client.on('close', function() {
      setImmediate(function() {
        console.log(client.ssl);
      });
    });
    server.close();
  });
});

Does the bhttp module also remove or shift the built-in close listener? Otherwise does it have a something else other reason?

@ChALkeR
Copy link
Member

ChALkeR commented Feb 10, 2016

Does the bhttp module also remove or shift the built-in close listener? Otherwise does it have a something else other reason?

/cc @joepie91 for bhttp.

@joepie91
Copy link
Contributor

I can't say that I fully understand the issue or how it is being resolved (as I'm not very familiar with Node core yet), but the bhttp code does monkeypatch some stuff in one place.

I'm not touching close events anywhere, though, unless one of the dependencies involved is doing so. There's also not currently any special handling of HTTPS - it simply uses the https module instead of the http module.

@indutny
Copy link
Member Author

indutny commented Feb 10, 2016

@shigeki according to @joepie91 it is, but anyway this is that simplest test case that I could come with. The idea behind the fix is correct too.

@shigeki
Copy link
Contributor

shigeki commented Feb 12, 2016

I've got that setImmediate causes this error as the bluebird module uses it as Promise scheduler.
When the callback of bhttp is invoked before destroySSL in the queues of setImmediate after the close event on TLSSocket, it leads the seg fault error. The error can be reproducible as

'use strict';
const common = require('../common');

if (!common.hasCrypto) {
  console.log('1..0 # Skipped: missing crypto');
  return;
}

const assert = require('assert');
const tls = require('tls');
const fs = require('fs');

const options = {
  key: fs.readFileSync(common.fixturesDir + '/keys/agent1-key.pem'),
  cert: fs.readFileSync(common.fixturesDir + '/keys/agent1-cert.pem')
};


const server = tls.createServer(options, function(s) {
  s.end('hello');
}).listen(common.PORT, function() {
  const opts = {port: common.PORT,
                rejectUnauthorized: false};
  const client = tls.connect(opts, function() {
    putImmediate(client);
  });
});


function putImmediate(client) {
  setImmediate(function() {
    if (client.ssl) {
      const fd = client.ssl.fd;
      assert(!!fd);
      putImmediate(client);
    } else {
      server.close();
    }
  });
}

The fix in this PR can resolve the issue. LGTM for a intermediate fix.

@indutny
Copy link
Member Author

indutny commented Feb 13, 2016

@shigeki do you mind if I will include your test? It seems to be much better than mine.

@shigeki
Copy link
Contributor

shigeki commented Feb 14, 2016

@indutny No problem for using the test. I reconsider this fix in this PR and had a question. With this patch, ssl.destorySSL() is not invoked so that an SSL object seems to be never freed. Does it lead to leaking? How about using process.nextTick(destroySSL, this); in stead of setImmediate?

client._events.close.unshift(common.mustCall(function() {
setImmediate(function() {
// Make it crash on unpatched node.js
var fd = client.ssl && client.ssl.fd;
Copy link
Contributor

Choose a reason for hiding this comment

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

If the intention is to fail the test here, why not assert it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because assert below will fail, and it won't crash.

@indutny
Copy link
Member Author

indutny commented Feb 16, 2016

Landed in aa05269, thank you!

@indutny indutny closed this Feb 16, 2016
@indutny indutny deleted the fix/gh-5108 branch February 16, 2016 19:13
indutny added a commit that referenced this pull request Feb 16, 2016
This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.

Proper fix is going to be submitted and landed after this one.

Fix: #5108
PR-URL: #5168
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
@ChALkeR
Copy link
Member

ChALkeR commented Feb 16, 2016

Thanks!

@ChALkeR ChALkeR mentioned this pull request Feb 16, 2016
rvagg pushed a commit that referenced this pull request Feb 18, 2016
This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.

Proper fix is going to be submitted and landed after this one.

Fix: #5108
PR-URL: #5168
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
stefanmb pushed a commit to stefanmb/node that referenced this pull request Feb 23, 2016
This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.

Proper fix is going to be submitted and landed after this one.

Fix: nodejs#5108
PR-URL: nodejs#5168
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.

Proper fix is going to be submitted and landed after this one.

Fix: #5108
PR-URL: #5168
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Mar 1, 2016
This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.

Proper fix is going to be submitted and landed after this one.

Fix: #5108
PR-URL: #5168
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
MylesBorins pushed a commit that referenced this pull request Mar 2, 2016
This is an intermediate fix for an issue of accessing `TLSWrap` fields
after the parent handle was destroyed. While `close` listener cleans up
this field automatically, it can be done even earlier at the
`TLSWrap.close` call.

Proper fix is going to be submitted and landed after this one.

Fix: #5108
PR-URL: #5168
Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Segfault in TLSWrap in v5.5.0 and master.
8 participants