Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

Commit 048e0be

Browse filesBrowse files
pimterrypimterry
authored
tls: ensure TLS Sockets are closed if the underlying wrap closes
This fixes a potential segfault, among various other likely-related issues, which all occur because TLSSockets were not informed if their underlying stream was closed in many cases. This also significantly modifies an existing TLS test. With this change in place, that test no longer works, as it tries to mess with internals to trigger a race, and those internals are now cleaned up earlier. This test has been simplified to a more general TLS shutdown test. PR-URL: #49327 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
1 parent 92fb7dd commit 048e0be
Copy full SHA for 048e0be

File tree

Expand file treeCollapse file tree

3 files changed

+91
-43
lines changed
Open diff view settings
Filter options
Expand file treeCollapse file tree

3 files changed

+91
-43
lines changed
Open diff view settings
Collapse file

‎lib/_tls_wrap.js‎

Copy file name to clipboardExpand all lines: lib/_tls_wrap.js
+3Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -704,6 +704,9 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle, wrapHasActiveWriteFromP
704704
defineHandleReading(this, handle);
705705

706706
this.on('close', onSocketCloseDestroySSL);
707+
if (wrap) {
708+
wrap.on('close', () => this.destroy());
709+
}
707710

708711
return res;
709712
};
Collapse file

‎test/parallel/test-http2-socket-close.js‎

Copy file name to clipboard
+67Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
'use strict';
2+
3+
const common = require('../common');
4+
const fixtures = require('../common/fixtures');
5+
if (!common.hasCrypto)
6+
common.skip('missing crypto');
7+
const assert = require('assert');
8+
const net = require('net');
9+
const h2 = require('http2');
10+
11+
const tlsOptions = {
12+
key: fixtures.readKey('agent1-key.pem'),
13+
cert: fixtures.readKey('agent1-cert.pem'),
14+
ALPNProtocols: ['h2']
15+
};
16+
17+
// Create a net server that upgrades sockets to HTTP/2 manually, handles the
18+
// request, and then shuts down via a short socket timeout and a longer H2 session
19+
// timeout. This is an unconventional way to shut down a session (the underlying
20+
// socket closing first) but it should work - critically, it shouldn't segfault
21+
// (as it did until Node v20.5.1).
22+
23+
let serverRawSocket;
24+
let serverH2Session;
25+
26+
const netServer = net.createServer((socket) => {
27+
serverRawSocket = socket;
28+
h2Server.emit('connection', socket);
29+
});
30+
31+
const h2Server = h2.createSecureServer(tlsOptions, (req, res) => {
32+
res.writeHead(200);
33+
res.end();
34+
});
35+
36+
h2Server.on('session', (session) => {
37+
serverH2Session = session;
38+
});
39+
40+
netServer.listen(0, common.mustCall(() => {
41+
const proxyClient = h2.connect(`https://localhost:${netServer.address().port}`, {
42+
rejectUnauthorized: false
43+
});
44+
45+
proxyClient.on('close', common.mustCall(() => {
46+
netServer.close();
47+
}));
48+
49+
const req = proxyClient.request({
50+
':method': 'GET',
51+
':path': '/'
52+
});
53+
54+
req.on('response', common.mustCall((response) => {
55+
assert.strictEqual(response[':status'], 200);
56+
57+
// Asynchronously shut down the server's connections after the response,
58+
// but not in the order it typically expects:
59+
setTimeout(() => {
60+
serverRawSocket.destroy();
61+
62+
setTimeout(() => {
63+
serverH2Session.close();
64+
}, 10);
65+
}, 10);
66+
}));
67+
}));
Collapse file

‎test/parallel/test-tls-socket-close.js‎

Copy file name to clipboardExpand all lines: test/parallel/test-tls-socket-close.js
+21-43Lines changed: 21 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -8,73 +8,51 @@ const tls = require('tls');
88
const net = require('net');
99
const fixtures = require('../common/fixtures');
1010

11-
// Regression test for https://github.com/nodejs/node/issues/8074
12-
//
13-
// This test has a dependency on the order in which the TCP connection is made,
14-
// and TLS server handshake completes. It assumes those server side events occur
15-
// before the client side write callback, which is not guaranteed by the TLS
16-
// API. It usually passes with TLS1.3, but TLS1.3 didn't exist at the time the
17-
// bug existed.
18-
//
19-
// Pin the test to TLS1.2, since the test shouldn't be changed in a way that
20-
// doesn't trigger a segfault in Node.js 7.7.3:
21-
// https://github.com/nodejs/node/issues/13184#issuecomment-303700377
22-
tls.DEFAULT_MAX_VERSION = 'TLSv1.2';
23-
2411
const key = fixtures.readKey('agent2-key.pem');
2512
const cert = fixtures.readKey('agent2-cert.pem');
2613

27-
let tlsSocket;
28-
// tls server
14+
let serverTlsSocket;
2915
const tlsServer = tls.createServer({ cert, key }, (socket) => {
30-
tlsSocket = socket;
31-
socket.on('error', common.mustCall((error) => {
32-
assert.strictEqual(error.code, 'EINVAL');
33-
tlsServer.close();
34-
netServer.close();
35-
}));
16+
serverTlsSocket = socket;
3617
});
3718

19+
// A plain net server, that manually passes connections to the TLS
20+
// server to be upgraded
3821
let netSocket;
39-
// plain tcp server
4022
const netServer = net.createServer((socket) => {
41-
// If client wants to use tls
4223
tlsServer.emit('connection', socket);
4324

4425
netSocket = socket;
4526
}).listen(0, common.mustCall(function() {
4627
connectClient(netServer);
4728
}));
4829

30+
// A client that connects, sends one message, and closes the raw connection:
4931
function connectClient(server) {
50-
const tlsConnection = tls.connect({
32+
const clientTlsSocket = tls.connect({
5133
host: 'localhost',
5234
port: server.address().port,
5335
rejectUnauthorized: false
5436
});
5537

56-
tlsConnection.write('foo', 'utf8', common.mustCall(() => {
38+
clientTlsSocket.write('foo', 'utf8', common.mustCall(() => {
5739
assert(netSocket);
5840
netSocket.setTimeout(common.platformTimeout(10), common.mustCall(() => {
59-
assert(tlsSocket);
60-
// This breaks if TLSSocket is already managing the socket:
41+
assert(serverTlsSocket);
42+
6143
netSocket.destroy();
62-
const interval = setInterval(() => {
63-
// Checking this way allows us to do the write at a time that causes a
64-
// segmentation fault (not always, but often) in Node.js 7.7.3 and
65-
// earlier. If we instead, for example, wait on the `close` event, then
66-
// it will not segmentation fault, which is what this test is all about.
67-
if (tlsSocket._handle._parent.bytesRead === 0) {
68-
tlsSocket.write('bar');
69-
clearInterval(interval);
70-
}
71-
}, 1);
44+
45+
setImmediate(() => {
46+
assert.strictEqual(netSocket.destroyed, true);
47+
assert.strictEqual(clientTlsSocket.destroyed, true);
48+
49+
setImmediate(() => {
50+
assert.strictEqual(serverTlsSocket.destroyed, true);
51+
52+
tlsServer.close();
53+
netServer.close();
54+
});
55+
});
7256
}));
7357
}));
74-
tlsConnection.on('error', (e) => {
75-
// Tolerate the occasional ECONNRESET.
76-
// Ref: https://github.com/nodejs/node/issues/13184
77-
if (e.code !== 'ECONNRESET')
78-
throw e;
79-
});
8058
}

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.