GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — net: fix crash due to simultaneous close/shutdown on JS Stream Sockets · nodejs/node@03063bd

File tree

Expand file treeCollapse file tree
Open diff view settings
Filter options
  • lib/internal
    • js_stream_socket.js
  • test/parallel
    • test-http2-client-connection-tunnelling.js
Expand file treeCollapse file tree
Open diff view settings
Collapse file

‎lib/internal/js_stream_socket.js‎

Copy file name to clipboardExpand all lines: lib/internal/js_stream_socket.js
+20Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ const { ERR_STREAM_WRAP } = require('internal/errors').codes;
2121
const kCurrentWriteRequest = Symbol('kCurrentWriteRequest');
2222
const kCurrentShutdownRequest = Symbol('kCurrentShutdownRequest');
2323
const kPendingShutdownRequest = Symbol('kPendingShutdownRequest');
24+
const kPendingClose = Symbol('kPendingClose');
2425

2526
function isClosing() { return this[owner_symbol].isClosing(); }
2627

@@ -94,6 +95,7 @@ class JSStreamSocket extends Socket {
9495
this[kCurrentWriteRequest] = null;
9596
this[kCurrentShutdownRequest] = null;
9697
this[kPendingShutdownRequest] = null;
98+
this[kPendingClose] = false;
9799
this.readable = stream.readable;
98100
this.writable = stream.writable;
99101

@@ -135,10 +137,17 @@ class JSStreamSocket extends Socket {
135137
this[kPendingShutdownRequest] = req;
136138
return 0;
137139
}
140+
138141
assert(this[kCurrentWriteRequest] === null);
139142
assert(this[kCurrentShutdownRequest] === null);
140143
this[kCurrentShutdownRequest] = req;
141144

145+
if (this[kPendingClose]) {
146+
// If doClose is pending, the stream & this._handle are gone. We can't do
147+
// anything. doClose will call finishShutdown with ECANCELED for us shortly.
148+
return 0;
149+
}
150+
142151
const handle = this._handle;
143152

144153
process.nextTick(() => {
@@ -164,6 +173,13 @@ class JSStreamSocket extends Socket {
164173
assert(this[kCurrentWriteRequest] === null);
165174
assert(this[kCurrentShutdownRequest] === null);
166175

176+
if (this[kPendingClose]) {
177+
// If doClose is pending, the stream & this._handle are gone. We can't do
178+
// anything. doClose will call finishWrite with ECANCELED for us shortly.
179+
this[kCurrentWriteRequest] = req; // Store req, for doClose to cancel
180+
return 0;
181+
}
182+
167183
const handle = this._handle;
168184
const self = this;
169185

@@ -217,6 +233,8 @@ class JSStreamSocket extends Socket {
217233
}
218234

219235
doClose(cb) {
236+
this[kPendingClose] = true;
237+
220238
const handle = this._handle;
221239

222240
// When sockets of the "net" module destroyed, they will call
@@ -234,6 +252,8 @@ class JSStreamSocket extends Socket {
234252
this.finishWrite(handle, uv.UV_ECANCELED);
235253
this.finishShutdown(handle, uv.UV_ECANCELED);
236254

255+
this[kPendingClose] = false;
256+
237257
cb();
238258
});
239259
}
Collapse file

‎test/parallel/test-http2-client-connection-tunnelling.js‎

Copy file name to clipboard
+71Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
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 tls = require('tls');
10+
const h2 = require('http2');
11+
12+
// This test sets up an H2 proxy server, and tunnels a request over one of its streams
13+
// back to itself, via TLS, and then closes the TLS connection. On some Node versions
14+
// (v18 & v20 up to 20.5.1) the resulting JS Stream Socket fails to shutdown correctly
15+
// in this case, and crashes due to a null pointer in finishShutdown.
16+
17+
const tlsOptions = {
18+
key: fixtures.readKey('agent1-key.pem'),
19+
cert: fixtures.readKey('agent1-cert.pem'),
20+
ALPNProtocols: ['h2']
21+
};
22+
23+
const netServer = net.createServer((socket) => {
24+
socket.allowHalfOpen = false;
25+
// ^ This allows us to trigger this reliably, but it's not strictly required
26+
// for the bug and crash to happen, skipping this just fails elsewhere later.
27+
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('connect', (req, res) => {
37+
res.writeHead(200, {});
38+
netServer.emit('connection', res.stream);
39+
});
40+
41+
netServer.listen(0, common.mustCall(() => {
42+
const proxyClient = h2.connect(`https://localhost:${netServer.address().port}`, {
43+
rejectUnauthorized: false
44+
});
45+
46+
const proxyReq = proxyClient.request({
47+
':method': 'CONNECT',
48+
':authority': 'example.com:443'
49+
});
50+
51+
proxyReq.on('response', common.mustCall((response) => {
52+
assert.strictEqual(response[':status'], 200);
53+
54+
// Create a TLS socket within the tunnel, and start sending a request:
55+
const tlsSocket = tls.connect({
56+
socket: proxyReq,
57+
ALPNProtocols: ['h2'],
58+
rejectUnauthorized: false
59+
});
60+
61+
proxyReq.on('close', common.mustCall(() => {
62+
proxyClient.close();
63+
netServer.close();
64+
}));
65+
66+
// Forcibly kill the TLS socket
67+
tlsSocket.destroy();
68+
69+
// This results in an async error in affected Node versions, before the 'close' event
70+
}));
71+
}));

0 commit comments

Comments
0 (0)
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.