Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

tls: do not hang without newSession handler #25739

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions lib/_tls_legacy.js
Original file line number Diff line number Diff line change
Expand Up @@ -662,14 +662,17 @@ function onnewsession(key, session) {
var self = this;
var once = false;

self.server.emit('newSession', key, session, function() {
if (!self.server.emit('newSession', key, session, done))
done();

function done() {
if (once)
return;
once = true;

if (self.ssl)
self.ssl.newSessionDone();
});
};
}


Expand Down
7 changes: 5 additions & 2 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,10 @@ function onnewsession(key, session) {
var once = false;

this._newSessionPending = true;
this.server.emit('newSession', key, session, function() {
if (!this.server.emit('newSession', key, session, done))
done();

function done() {
if (once)
return;
once = true;
Expand All @@ -212,7 +215,7 @@ function onnewsession(key, session) {
if (self._securePending)
self._finishInit();
self._securePending = false;
});
}
}


Expand Down
42 changes: 42 additions & 0 deletions test/simple/test-tls-new-session-hang.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
var common = require('../common');

if (!process.features.tls_ocsp) {
console.error('Skipping because node compiled without OpenSSL or ' +
'with old OpenSSL version.');
process.exit(0);
}

var assert = require('assert');
var tls = require('tls');
var constants = require('constants');
var fs = require('fs');
var join = require('path').join;

var keyFile = join(common.fixturesDir, 'keys', 'agent1-key.pem');
var certFile = join(common.fixturesDir, 'keys', 'agent1-cert.pem');
var caFile = join(common.fixturesDir, 'keys', 'ca1-cert.pem');
var key = fs.readFileSync(keyFile);
var cert = fs.readFileSync(certFile);

var server = tls.createServer({
cert: cert,
key: key
}, function (socket) {
socket.destroySoon();
});

server.on('resumeSession', common.mustCall(function() {
// Should not be actually called
}, 0));

server.listen(common.PORT, function() {
var client = tls.connect({
rejectUnauthorized: false,
port: common.PORT,

// Just to make sure that `newSession` is going to be called
secureOptions: constants.SSL_OP_NO_TICKET
}, function() {
server.close();
});
});
28 changes: 21 additions & 7 deletions test/simple/test-tls-ocsp-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,16 +31,19 @@ if (!common.opensslCli) {
process.exit(0);
}

var assert = require('assert');
var tls = require('tls');
var constants = require('constants');
var fs = require('fs');
var join = require('path').join;

test({ response: false }, function() {
test({ response: 'hello world' });
test({ response: 'hello world' }, function() {
test({ ocsp: false });
});
});

function test(testOptions, cb) {
var assert = require('assert');
var tls = require('tls');
var fs = require('fs');
var join = require('path').join;
var spawn = require('child_process').spawn;

var keyFile = join(common.fixturesDir, 'keys', 'agent1-key.pem');
var certFile = join(common.fixturesDir, 'keys', 'agent1-cert.pem');
Expand All @@ -54,6 +57,7 @@ function test(testOptions, cb) {
ca: [ca]
};
var requestCount = 0;
var clientSecure = 0;
var ocspCount = 0;
var ocspResponse;
var session;
Expand Down Expand Up @@ -83,9 +87,12 @@ function test(testOptions, cb) {
server.listen(common.PORT, function() {
var client = tls.connect({
port: common.PORT,
requestOCSP: true,
requestOCSP: testOptions.ocsp !== false,
secureOptions: testOptions.ocsp === false ?
constants.SSL_OP_NO_TICKET : 0,
rejectUnauthorized: false
}, function() {
clientSecure++;
});
client.on('OCSPResponse', function(resp) {
ocspResponse = resp;
Expand All @@ -98,12 +105,19 @@ function test(testOptions, cb) {
});

process.on('exit', function() {
if (testOptions.ocsp === false) {
assert.equal(requestCount, clientSecure);
assert.equal(requestCount, 1);

Choose a reason for hiding this comment

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

I would use common.mustCall to do the number of invocation checks.

return;
}

if (testOptions.response) {
assert.equal(ocspResponse.toString(), testOptions.response);
} else {
assert.ok(ocspResponse === null);
}
assert.equal(requestCount, testOptions.response ? 0 : 1);
assert.equal(clientSecure, requestCount);
assert.equal(ocspCount, 1);
});
}