Skip to content

Commit

Permalink
tls: emit errors on close whilst async action
Browse files Browse the repository at this point in the history
When loading session, OCSP response, SNI, always check that the
`self._handle` is present. If it is not - the socket was closed - and we
should emit the error instead of throwing an uncaught exception.

Fix: nodejs/node-v0.x-archive#8780
Fix: #1696
PR-URL: #1702
Reviewed-By: Trevor Norris <[email protected]>
  • Loading branch information
indutny committed Jun 4, 2015
1 parent 6d95f4f commit 5795e83
Show file tree
Hide file tree
Showing 2 changed files with 88 additions and 0 deletions.
15 changes: 15 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ function loadSession(self, hello, cb) {
if (err)
return cb(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

// NOTE: That we have disabled OpenSSL's internal session storage in
// `node_crypto.cc` and hence its safe to rely on getting servername only
// from clienthello or this place.
Expand Down Expand Up @@ -91,6 +94,9 @@ function loadSNI(self, servername, cb) {
if (err)
return cb(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

// TODO(indutny): eventually disallow raw `SecureContext`
if (context)
self._handle.sni_context = context.context || context;
Expand Down Expand Up @@ -127,6 +133,9 @@ function requestOCSP(self, hello, ctx, cb) {
if (err)
return cb(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

if (response)
self._handle.setOCSPResponse(response);
cb(null);
Expand Down Expand Up @@ -157,6 +166,9 @@ function oncertcb(info) {
if (err)
return self.destroy(err);

if (!self._handle)
return cb(new Error('Socket is closed'));

This comment has been minimized.

Copy link
@silverwind

silverwind Jun 11, 2015

Contributor

@indutny cb is undefined here. This just came up in my linter with the no-undef rule enabled.

This comment has been minimized.

Copy link
@indutny

indutny Jun 11, 2015

Author Member

True, let me fix it.

self._handle.certCbDone();
});
});
Expand All @@ -179,6 +191,9 @@ function onnewsession(key, session) {
return;
once = true;

if (!self._handle)
return cb(new Error('Socket is closed'));

This comment has been minimized.

Copy link
@silverwind

silverwind Jun 11, 2015

Contributor

Same here, cb is undefined.

self._handle.newSessionDone();

self._newSessionPending = false;
Expand Down
73 changes: 73 additions & 0 deletions test/parallel/test-tls-async-cb-after-socket-end.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
'use strict';

var common = require('../common');

var assert = require('assert');
var path = require('path');
var fs = require('fs');
var constants = require('constants');

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

var tls = require('tls');

var options = {
secureOptions: constants.SSL_OP_NO_TICKET,
key: fs.readFileSync(path.join(common.fixturesDir, 'test_key.pem')),
cert: fs.readFileSync(path.join(common.fixturesDir, 'test_cert.pem'))
};

var server = tls.createServer(options, function(c) {
});

var sessionCb = null;
var client = null;

server.on('newSession', function(key, session, done) {
done();
});

server.on('resumeSession', function(id, cb) {
sessionCb = cb;

next();
});

server.listen(1443, function() {
var clientOpts = {
port: 1443,
rejectUnauthorized: false,
session: false
};

var s1 = tls.connect(clientOpts, function() {
clientOpts.session = s1.getSession();
console.log('1st secure');

s1.destroy();
var s2 = tls.connect(clientOpts, function(s) {
console.log('2nd secure');

s2.destroy();
}).on('connect', function() {
console.log('2nd connected');
client = s2;

next();
});
});
});

function next() {
if (!client || !sessionCb)
return;

client.destroy();
setTimeout(function() {
sessionCb();
server.close();
}, 100);
}

0 comments on commit 5795e83

Please sign in to comment.