-
Notifications
You must be signed in to change notification settings - Fork 15k
/
tls_ensure_tls_sockets_are_closed_if_the_underlying_wrap_closes.patch
204 lines (192 loc) · 6.93 KB
/
tls_ensure_tls_sockets_are_closed_if_the_underlying_wrap_closes.patch
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
176
177
178
179
180
181
182
183
184
185
186
187
188
189
190
191
192
193
194
195
196
197
198
199
200
201
202
203
204
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: Tim Perry <1526883+pimterry@users.noreply.github.com>
Date: Fri, 1 Sep 2023 09:00:05 +0200
Subject: 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: https://github.com/nodejs/node/pull/49327
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js
index 84599102be4b74ff66af63c36c232f4656a1f406..bb0535558a8999034a99d0e7e99507ba10eca2eb 100644
--- a/lib/_tls_wrap.js
+++ b/lib/_tls_wrap.js
@@ -661,6 +661,9 @@ TLSSocket.prototype._wrapHandle = function(wrap, handle, wrapHasActiveWriteFromP
defineHandleReading(this, handle);
this.on('close', onSocketCloseDestroySSL);
+ if (wrap) {
+ wrap.on('close', () => this.destroy());
+ }
return res;
};
diff --git a/test/parallel/test-http2-socket-close.js b/test/parallel/test-http2-socket-close.js
new file mode 100644
index 0000000000000000000000000000000000000000..02db77bcf8480c79c77175ba802f9fe10ffc4efe
--- /dev/null
+++ b/test/parallel/test-http2-socket-close.js
@@ -0,0 +1,67 @@
+'use strict';
+
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+if (!common.hasCrypto)
+ common.skip('missing crypto');
+const assert = require('assert');
+const net = require('net');
+const h2 = require('http2');
+
+const tlsOptions = {
+ key: fixtures.readKey('agent1-key.pem'),
+ cert: fixtures.readKey('agent1-cert.pem'),
+ ALPNProtocols: ['h2']
+};
+
+// Create a net server that upgrades sockets to HTTP/2 manually, handles the
+// request, and then shuts down via a short socket timeout and a longer H2 session
+// timeout. This is an unconventional way to shut down a session (the underlying
+// socket closing first) but it should work - critically, it shouldn't segfault
+// (as it did until Node v20.5.1).
+
+let serverRawSocket;
+let serverH2Session;
+
+const netServer = net.createServer((socket) => {
+ serverRawSocket = socket;
+ h2Server.emit('connection', socket);
+});
+
+const h2Server = h2.createSecureServer(tlsOptions, (req, res) => {
+ res.writeHead(200);
+ res.end();
+});
+
+h2Server.on('session', (session) => {
+ serverH2Session = session;
+});
+
+netServer.listen(0, common.mustCall(() => {
+ const proxyClient = h2.connect(`https://localhost:${netServer.address().port}`, {
+ rejectUnauthorized: false
+ });
+
+ proxyClient.on('close', common.mustCall(() => {
+ netServer.close();
+ }));
+
+ const req = proxyClient.request({
+ ':method': 'GET',
+ ':path': '/'
+ });
+
+ req.on('response', common.mustCall((response) => {
+ assert.strictEqual(response[':status'], 200);
+
+ // Asynchronously shut down the server's connections after the response,
+ // but not in the order it typically expects:
+ setTimeout(() => {
+ serverRawSocket.destroy();
+
+ setTimeout(() => {
+ serverH2Session.close();
+ }, 10);
+ }, 10);
+ }));
+}));
diff --git a/test/parallel/test-tls-socket-close.js b/test/parallel/test-tls-socket-close.js
index 87355cf8d7bd2d07bb0fab59491b68f3963f8809..667b291309a4c5636a2c658fa8204b32c2e4df46 100644
--- a/test/parallel/test-tls-socket-close.js
+++ b/test/parallel/test-tls-socket-close.js
@@ -8,37 +8,18 @@ const tls = require('tls');
const net = require('net');
const fixtures = require('../common/fixtures');
-// Regression test for https://github.com/nodejs/node/issues/8074
-//
-// This test has a dependency on the order in which the TCP connection is made,
-// and TLS server handshake completes. It assumes those server side events occur
-// before the client side write callback, which is not guaranteed by the TLS
-// API. It usually passes with TLS1.3, but TLS1.3 didn't exist at the time the
-// bug existed.
-//
-// Pin the test to TLS1.2, since the test shouldn't be changed in a way that
-// doesn't trigger a segfault in Node.js 7.7.3:
-// https://github.com/nodejs/node/issues/13184#issuecomment-303700377
-tls.DEFAULT_MAX_VERSION = 'TLSv1.2';
-
const key = fixtures.readKey('agent2-key.pem');
const cert = fixtures.readKey('agent2-cert.pem');
-let tlsSocket;
-// tls server
+let serverTlsSocket;
const tlsServer = tls.createServer({ cert, key }, (socket) => {
- tlsSocket = socket;
- socket.on('error', common.mustCall((error) => {
- assert.strictEqual(error.code, 'EINVAL');
- tlsServer.close();
- netServer.close();
- }));
+ serverTlsSocket = socket;
});
+// A plain net server, that manually passes connections to the TLS
+// server to be upgraded
let netSocket;
-// plain tcp server
const netServer = net.createServer((socket) => {
- // If client wants to use tls
tlsServer.emit('connection', socket);
netSocket = socket;
@@ -46,35 +27,32 @@ const netServer = net.createServer((socket) => {
connectClient(netServer);
}));
+// A client that connects, sends one message, and closes the raw connection:
function connectClient(server) {
- const tlsConnection = tls.connect({
+ const clientTlsSocket = tls.connect({
host: 'localhost',
port: server.address().port,
rejectUnauthorized: false
});
- tlsConnection.write('foo', 'utf8', common.mustCall(() => {
+ clientTlsSocket.write('foo', 'utf8', common.mustCall(() => {
assert(netSocket);
netSocket.setTimeout(common.platformTimeout(10), common.mustCall(() => {
- assert(tlsSocket);
- // This breaks if TLSSocket is already managing the socket:
+ assert(serverTlsSocket);
+
netSocket.destroy();
- const interval = setInterval(() => {
- // Checking this way allows us to do the write at a time that causes a
- // segmentation fault (not always, but often) in Node.js 7.7.3 and
- // earlier. If we instead, for example, wait on the `close` event, then
- // it will not segmentation fault, which is what this test is all about.
- if (tlsSocket._handle._parent.bytesRead === 0) {
- tlsSocket.write('bar');
- clearInterval(interval);
- }
- }, 1);
+
+ setImmediate(() => {
+ assert.strictEqual(netSocket.destroyed, true);
+ assert.strictEqual(clientTlsSocket.destroyed, true);
+
+ setImmediate(() => {
+ assert.strictEqual(serverTlsSocket.destroyed, true);
+
+ tlsServer.close();
+ netServer.close();
+ });
+ });
}));
}));
- tlsConnection.on('error', (e) => {
- // Tolerate the occasional ECONNRESET.
- // Ref: https://github.com/nodejs/node/issues/13184
- if (e.code !== 'ECONNRESET')
- throw e;
- });
}