Skip to content

Commit e7be7bd

Browse files
committed
tls: propagate singleUse to the secure context
tls.connect() sets options.singleUse, and both TLSWrap.prototype.close() and TLSSocket.prototype._destroySSL() check _secureContext.singleUse before closing the context. Nothing ever assigned the flag to the context, so the check never fired and each connection's SSL_CTX stayed alive until the JS wrapper was garbage collected. The assignment was lost when the context configuration was extracted into configSecureContext(). Restore it in createSecureContext(), which owns the JS wrapper the flag is read from, and make the close idempotent: a socket that swaps its handle, as autoSelectFamily retries do, shares a single context between the successive TLSWraps and each of them reaches the same teardown. Signed-off-by: Carlos Vinicius <viniciusdev.26@gmail.com>
1 parent c9eed87 commit e7be7bd

3 files changed

Lines changed: 95 additions & 8 deletions

File tree

lib/internal/tls/common.js

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ function createSecureContext(options) {
101101
minVersion,
102102
maxVersion,
103103
secureProtocol,
104+
singleUse,
104105
} = options;
105106

106107
let { secureOptions } = options;
@@ -113,6 +114,13 @@ function createSecureContext(options) {
113114

114115
configSecureContext(c.context, options);
115116

117+
// A single-use context belongs to exactly one connection, so the underlying
118+
// SSL_CTX can be freed as soon as that connection closes instead of waiting
119+
// for the JS wrapper to be garbage collected. TLSSocket relies on this flag
120+
// to know that it may close the context itself.
121+
if (singleUse)
122+
c.singleUse = true;
123+
116124
return c;
117125
}
118126

lib/internal/tls/wrap.js

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -748,6 +748,19 @@ for (const proxiedMethod of proxiedMethods) {
748748
makeMethodProxy(proxiedMethod);
749749
}
750750

751+
// A single-use context is owned by the connection that created it, so its
752+
// SSL_CTX can be released as soon as the connection is gone instead of
753+
// waiting for the JS wrapper to be garbage collected. A socket that swaps
754+
// its handle (autoSelectFamily retries, kReinitializeHandle) shares one
755+
// context between the successive TLSWraps, and the teardown may be reached
756+
// from more than one path, so closing has to be idempotent.
757+
function closeSingleUseContext(secureContext) {
758+
if (secureContext.singleUse && secureContext.context !== null) {
759+
secureContext.context.close();
760+
secureContext.context = null;
761+
}
762+
}
763+
751764
tls_wrap.TLSWrap.prototype.close = function close(cb) {
752765
let ssl;
753766
if (this[owner_symbol]) {
@@ -760,10 +773,7 @@ tls_wrap.TLSWrap.prototype.close = function close(cb) {
760773
const done = () => {
761774
if (ssl) {
762775
ssl.destroySSL();
763-
if (ssl._secureContext.singleUse) {
764-
ssl._secureContext.context.close();
765-
ssl._secureContext.context = null;
766-
}
776+
closeSingleUseContext(ssl._secureContext);
767777
}
768778
if (cb)
769779
cb();
@@ -888,10 +898,7 @@ function destroySSL(self) {
888898
TLSSocket.prototype._destroySSL = function _destroySSL() {
889899
if (!this.ssl) return;
890900
this.ssl.destroySSL();
891-
if (this.ssl._secureContext.singleUse) {
892-
this.ssl._secureContext.context.close();
893-
this.ssl._secureContext.context = null;
894-
}
901+
closeSingleUseContext(this.ssl._secureContext);
895902
this.ssl = null;
896903
this[kPendingSession] = null;
897904
this[kIsVerified] = false;
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
8+
// tls.connect() builds a SecureContext per connection and marks it as single
9+
// use, so that the underlying SSL_CTX is released as soon as the socket
10+
// closes instead of when the JS wrapper happens to be garbage collected.
11+
// Regression test for https://github.com/nodejs/node/issues/66002, where the
12+
// flag was lost on its way to the context and nothing was ever closed.
13+
14+
const assert = require('assert');
15+
const tls = require('tls');
16+
const fixtures = require('../common/fixtures');
17+
18+
const key = fixtures.readKey('agent1-key.pem');
19+
const cert = fixtures.readKey('agent1-cert.pem');
20+
21+
const server = tls.createServer({ key, cert }, (conn) => conn.end());
22+
23+
server.listen(0, common.mustCall(() => {
24+
connectWithOwnContext(common.mustCall(() => {
25+
connectWithSharedContext(common.mustCall(() => server.close()));
26+
}));
27+
}));
28+
29+
// _destroySSL() runs from the immediate queue, after the 'close' event.
30+
function afterDestroySSL(socket, fn) {
31+
socket.on('close', common.mustCall(() => setImmediate(fn)));
32+
}
33+
34+
function connectWithOwnContext(done) {
35+
const socket = tls.connect({
36+
port: server.address().port,
37+
rejectUnauthorized: false,
38+
}, common.mustCall(() => {
39+
const secureContext = socket.ssl._secureContext;
40+
assert.strictEqual(secureContext.singleUse, true);
41+
assert.notStrictEqual(secureContext.context, null);
42+
43+
afterDestroySSL(socket, common.mustCall(() => {
44+
assert.strictEqual(secureContext.context, null);
45+
done();
46+
}));
47+
}));
48+
}
49+
50+
function connectWithSharedContext(done) {
51+
// A context passed in by the user may outlive the connection, so it must
52+
// not be marked single use, and it must still work for the next socket.
53+
const secureContext = tls.createSecureContext();
54+
let remaining = 2;
55+
56+
(function connectOnce() {
57+
const socket = tls.connect({
58+
port: server.address().port,
59+
rejectUnauthorized: false,
60+
secureContext,
61+
}, common.mustCall(() => {
62+
assert.strictEqual(socket.ssl._secureContext, secureContext);
63+
assert.strictEqual(secureContext.singleUse, undefined);
64+
65+
afterDestroySSL(socket, common.mustCall(() => {
66+
assert.notStrictEqual(secureContext.context, null);
67+
if (--remaining === 0) done();
68+
else connectOnce();
69+
}));
70+
}));
71+
})();
72+
}

0 commit comments

Comments
 (0)