Skip to content

Commit

Permalink
net: fix crash due to simultaneous close/shutdown on JS Stream Sockets
Browse files Browse the repository at this point in the history
A JS stream socket wraps a stream, exposing it as a socket for something
on top which needs a socket specifically (e.g. an HTTP server).

If the internal stream is closed in the same tick as the layer on top
attempts to close this stream, the race between doShutdown and doClose
results in an uncatchable exception. A similar race can happen with
doClose and doWrite.

It seems legitimate these can happen in parallel, so this resolves that
by explicitly detecting and handling that situation: if a close is in
progress, both doShutdown & doWrite allow doClose to run
finishShutdown/Write for them, cancelling the operation, without trying
to use this._handle (which will be null) in the meantime.

PR-URL: #49400
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
  • Loading branch information
pimterry authored and UlisesGascon committed Sep 10, 2023
1 parent e787673 commit 2486836
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 0 deletions.
20 changes: 20 additions & 0 deletions lib/internal/js_stream_socket.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ const { ERR_STREAM_WRAP } = require('internal/errors').codes;
const kCurrentWriteRequest = Symbol('kCurrentWriteRequest');
const kCurrentShutdownRequest = Symbol('kCurrentShutdownRequest');
const kPendingShutdownRequest = Symbol('kPendingShutdownRequest');
const kPendingClose = Symbol('kPendingClose');

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

Expand Down Expand Up @@ -94,6 +95,7 @@ class JSStreamSocket extends Socket {
this[kCurrentWriteRequest] = null;
this[kCurrentShutdownRequest] = null;
this[kPendingShutdownRequest] = null;
this[kPendingClose] = false;
this.readable = stream.readable;
this.writable = stream.writable;

Expand Down Expand Up @@ -135,10 +137,17 @@ class JSStreamSocket extends Socket {
this[kPendingShutdownRequest] = req;
return 0;
}

assert(this[kCurrentWriteRequest] === null);
assert(this[kCurrentShutdownRequest] === null);
this[kCurrentShutdownRequest] = req;

if (this[kPendingClose]) {
// If doClose is pending, the stream & this._handle are gone. We can't do
// anything. doClose will call finishShutdown with ECANCELED for us shortly.
return 0;
}

const handle = this._handle;

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

if (this[kPendingClose]) {
// If doClose is pending, the stream & this._handle are gone. We can't do
// anything. doClose will call finishWrite with ECANCELED for us shortly.
this[kCurrentWriteRequest] = req; // Store req, for doClose to cancel
return 0;
}

const handle = this._handle;
const self = this;

Expand Down Expand Up @@ -217,6 +233,8 @@ class JSStreamSocket extends Socket {
}

doClose(cb) {
this[kPendingClose] = true;

const handle = this._handle;

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

this[kPendingClose] = false;

cb();
});
}
Expand Down
71 changes: 71 additions & 0 deletions test/parallel/test-http2-client-connection-tunnelling.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
'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 tls = require('tls');
const h2 = require('http2');

// This test sets up an H2 proxy server, and tunnels a request over one of its streams
// back to itself, via TLS, and then closes the TLS connection. On some Node versions
// (v18 & v20 up to 20.5.1) the resulting JS Stream Socket fails to shutdown correctly
// in this case, and crashes due to a null pointer in finishShutdown.

const tlsOptions = {
key: fixtures.readKey('agent1-key.pem'),
cert: fixtures.readKey('agent1-cert.pem'),
ALPNProtocols: ['h2']
};

const netServer = net.createServer((socket) => {
socket.allowHalfOpen = false;
// ^ This allows us to trigger this reliably, but it's not strictly required
// for the bug and crash to happen, skipping this just fails elsewhere later.

h2Server.emit('connection', socket);
});

const h2Server = h2.createSecureServer(tlsOptions, (req, res) => {
res.writeHead(200);
res.end();
});

h2Server.on('connect', (req, res) => {
res.writeHead(200, {});
netServer.emit('connection', res.stream);
});

netServer.listen(0, common.mustCall(() => {
const proxyClient = h2.connect(`https://localhost:${netServer.address().port}`, {
rejectUnauthorized: false
});

const proxyReq = proxyClient.request({
':method': 'CONNECT',
':authority': 'example.com:443'
});

proxyReq.on('response', common.mustCall((response) => {
assert.strictEqual(response[':status'], 200);

// Create a TLS socket within the tunnel, and start sending a request:
const tlsSocket = tls.connect({
socket: proxyReq,
ALPNProtocols: ['h2'],
rejectUnauthorized: false
});

proxyReq.on('close', common.mustCall(() => {
proxyClient.close();
netServer.close();
}));

// Forcibly kill the TLS socket
tlsSocket.destroy();

// This results in an async error in affected Node versions, before the 'close' event
}));
}));

0 comments on commit 2486836

Please sign in to comment.