stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams #35904

mscdex · 2020-10-31T21:02:01Z

Version: v12.16.3+, v14.0.0+, v15.0.0+, master
Platform: n/a
Subsystem: stream

What steps will reproduce the bug?

The following code needs a private key and certificate for an HTTPS server.

const { readFileSync } = require('fs');
const tls = require('tls');
const https = require('https');
const net = require('net');
const { Duplex } = require('stream');

class CustomAgent extends https.Agent {
  constructor() {
    super();
  }
  createConnection(options, cb) {
    const realSocket = net.createConnection(options);
    const stream = new Duplex({
      emitClose: false,
      read(n) {
        (function retry() {
          const data = realSocket.read();
          if (data === null)
            return realSocket.once('readable', retry);
          stream.push(data);
        })();
      },
      write(chunk, enc, callback) {
        realSocket.write(chunk, enc, callback);
      },
    });
    realSocket.on('end', () => stream.push(null));

    stream.on('end', () => {
      console.log('stream end');
    }).on('close', () => {
      console.log('stream close');
    });

    return tls.connect({ ...options, socket: stream });
  }
}

const httpsServer = https.createServer({
  key: readFileSync('https_key.pem'),
  cert: readFileSync('https_cert.pem'),
}, (req, res) => {
  httpsServer.close();
  res.end('hello world!');
});
httpsServer.listen(0, 'localhost', () => {
  const agent = new CustomAgent();
  https.get({
    host: 'localhost',
    port: httpsServer.address().port,
    agent,
    headers: { Connection: 'close' },
    ca: readFileSync('https_cert.pem'),
  }, (res) => {
    res.resume();
  });
});

How often does it reproduce? Is there a required condition?

Every time.

What is the expected behavior?

"stream end" should be displayed

What do you see instead?

Nothing is displayed.

Additional information

This was originally discovered while adding tests for custom HTTP and HTTPS agents for the ssh2 module. I believe the key here is mainly using a custom stream for the socket passed to tls.connect(). However the regression only became evident once I started setting emitClose: false because my agent implementation was relying on the 'close' event being emitted to know when to close the underlying ssh connection and emitting 'close' is expected to be handled by ssh2 (because the protocol has an explicit close message separate from "EOF").

Bisecting reveals ed21d32 as the bad commit. What's happening is that when TLSSocket sees that the socket option is some custom object/stream and not a net.Socket instance, it wraps the stream with a JSStreamSocket. JSStreamSocket happens to have a readStop() implementation that simply pauses the socket. Since ed21d32 added a check for this method's existence, it now gets called when the TLS portion ends, which means the underlying/original stream now stays paused and will never emit 'end' like it used to.

Judging by the added code comments, it appears that this change was made to appease some error on Windows. I think the original error should be solved in a more compatible way.

/cc @ronag @addaleax @lpinca

The text was updated successfully, but these errors were encountered:

mmomtchev · 2020-11-03T14:13:24Z

I confirm that removing the kludge with the handle.readStop() fixes your problem, so maybe this issue should be mostly about why is that kludge needed in the first place. My opinion, is that is masks an incorrect Windows behavior. The ECONNRESET is not caused by an extra call to onStreamRead() - there is really a TCP RST packet that gets sent from the server to the client (yeah, it is weird). Calling handle.readStop() causes the stream to ignore that packet, thus masking the problem. I am not great with Windows sockets, is there someone with good understanding of Windows TCP/IP who can trace what calls does TLS server / libuv make that cause Windows to sent a TCP RST packet?

mmomtchev · 2020-11-03T14:35:34Z

@ronag, maybe add an 'error' handler to the https-truncate test until there is a fix/workaround for TLS/libuv? The RST on Windows is absolutely real, it is not an artifact of the stream code.

Remove the kludge that masks the TCP RST on Windows on test-https-truncate That RST is very real, originates from the server and should be investigated Fixes: nodejs#35904

mmomtchev · 2020-11-03T16:00:01Z

This is my proposal and maybe create a separate issue for the RST on Windows

On Windows and OSX, the HTTP server will often make the OS send a TCP RST packet by shutting down the socket before all incoming data has been read. Normally, libuv should take care of this (?), but it does not on Windows and OSX Refs: nodejs#35904

The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expect behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL:

This is an attempt to fix some resource management issues on Windows. Win32 sockets has a bug where it sends an RST packet if there is an outstanding overlapped WSARecv call. We can avoid that by being certain to explicitly cancel our read request first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expect behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL:

mmomtchev · 2020-11-05T21:35:46Z

The libuv team is working on a correct solution for test-https-truncate but given the looks of it, it won't make it anytime soon in Node
I wonder if the best solution wouldn't be to remove the kludge masking the RST packet on Windows and add an 'error' handler to test-https-truncate now that we know what causes the connection reset and that it can be safely ignored (for that test)
There is just one more RST on OSX that must be investigated - it seems very similar to the Windows one - caused by res.end()

This is an attempt to fix some resource management issues on Windows. Win32 sockets have an issue where it sends an RST packet if there is an outstanding overlapped calls. We can avoid that by being certain to explicitly cancel our read and write requests first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expected behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL: libuv#3036

RFC 5246 section-7.2.1 requires that the implementation must immediately stop using the stream, as it is no longer TLS-encrypted. The stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do remove ourselves as a listener. The section also states that the application must destroy the stream immediately (discarding any pending writes, and sending a close_notify response back), but we leave that to the upper layer of the application here, as it should be sufficient to permit standards compliant usage just to be ignoring read events. Fixes: nodejs/node#35904 Closes: nodejs/node#35946 Co-authored-by: Momtchil Momtchev <[email protected]>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in nodejs#35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. Refs: libuv/libuv#3036 Refs: nodejs#35904 Closes: nodejs#35946 Co-authored-by: Momtchil Momtchev <[email protected]>

This is an attempt to fix some resource management issues on Windows. Win32 sockets have an issue where it sends an RST packet if there is an outstanding overlapped calls. We can avoid that by being certain to explicitly cancel our read and write requests first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expected behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Additionally, we need to cancel writes explicitly: we need to notify Win32 that it is okay to cancel these writes (so it doesn't also generate an RST packet on the wire). Refs: #3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: #3034 PR-URL: #3036 Reviewed-By: Santiago Gimeno <[email protected]>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in nodejs#35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. Refs: libuv/libuv#3036 Refs: nodejs#35904 Closes: nodejs#35946 Co-authored-by: Momtchil Momtchev <[email protected]>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in #35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. PR-URL: #36111 Fixes: #35946 Refs: libuv/libuv#3036 Refs: #35904 Co-authored-by: Momtchil Momtchev <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in nodejs#35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. PR-URL: nodejs#36111 Fixes: nodejs#35946 Refs: libuv/libuv#3036 Refs: nodejs#35904 Co-authored-by: Momtchil Momtchev <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

RFC 5246 section-7.2.1 requires that the implementation must immediately stop reading from the stream, as it is no longer TLS-encrypted. The underlying stream is permitted to still pump events (and errors) to other users, but those are now unencrypted, so we should not process them here. But therefore, we do not want to stop the underlying stream, as there could be another user of it, but we do need to remove ourselves as a listener. Per TLS v1.2, we should have also destroy the TLS state entirely here (including the writing side), but this was revised in TLS v1.3 to permit the stream to continue to flush output. There appears to be some inconsistencies in the way nodejs handles ownership of the underlying stream, with `TLS.close()` on the write side also calling shutdown on the underlying stream (thus assuming other users of the underlying stream are not permitted), while receiving EOF on the read side leaves the underlying channel open. These inconsistencies are left for a later person to resolve, if the extra functionality is needed (as described in #35904). The current goal here is to the fix the occasional CI exceptions depending on the timing of these kernel messages through the TCP stack. PR-URL: #36111 Fixes: #35946 Refs: libuv/libuv#3036 Refs: #35904 Co-authored-by: Momtchil Momtchev <[email protected]> Reviewed-By: Robert Nagy <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Mesteery · 2022-02-17T22:43:27Z

Is it fixed?

This is an attempt to fix some resource management issues on Windows. Win32 sockets have an issue where it sends an RST packet if there is an outstanding overlapped calls. We can avoid that by being certain to explicitly cancel our read and write requests first. This also removes some conditional cleanup code, since we might as well clean it up eagerly (like unix). Otherwise, it looks to me like these might cause the accept callbacks to be run after the endgame had freed the memory for them. The comment here seems mixed up between send and recv buffers. The default behavior on calling `closesocket` is already to do a graceful shutdown (see https://docs.microsoft.com/en-us/windows/win32/api/winsock/nf-winsock-closesocket with default l_onoff=zero) if it is the last open handle. The expected behavior if there are pending reads in flight is to send an RST packet, notifying the client that the server connection was destroyed before acknowledging the EOF. Additionally, we need to cancel writes explicitly: we need to notify Win32 that it is okay to cancel these writes (so it doesn't also generate an RST packet on the wire). Refs: libuv#3035 Refs: nodejs/node#35946 Refs: nodejs/node#35904 Fixes: libuv#3034 PR-URL: libuv#3036 Reviewed-By: Santiago Gimeno <[email protected]>

octogonz · 2022-07-05T18:59:25Z

Is it fixed?

I'm having trouble following the bookkeeping as well. The description for PR #36111 say that it "closes" PR #35946. And PR #35946 had a description saying that it "fixes" this issue #35904. Does that imply that issue #35904 is now fixed? Then why is it still open?

@vtjnash could you clarify?

vtjnash · 2022-07-05T19:11:37Z

From #36111:

[This PR] does not address new feature #35904

The feature requested in the code at the top of this issue has not been implemented in nodejs. I do not know if the nodejs developers want to support this feature (permitting continuing to use using the unencrypted stream after the TLS-encrypted portion is done) or not. Currently nodejs expects to destroy the underlying (unencrypted) stream as soon as the encrypted (TLS) portion ends.

octogonz · 2022-07-05T19:16:08Z

So if it is a feature request, why is it tagged as a "regression"?

vtjnash · 2022-07-05T19:22:01Z

I have no control over tags

octogonz · 2022-07-05T19:25:24Z

👍 Thanks very much for clarifying, very helpful!

mscdex added regression Issues related to regressions. stream Issues and PRs related to the stream subsystem. labels Oct 31, 2020

mscdex changed the title ~~stream: regression since v12.16.3~~ stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams Oct 31, 2020

mmomtchev mentioned this issue Nov 3, 2020

stream: remove kludge masking RST on Windows #35946

Closed

3 tasks

mmomtchev mentioned this issue Nov 4, 2020

Closing a socket with a pending read makes Windows send a TCP RST packet libuv/libuv#3034

Closed

vtjnash mentioned this issue Nov 13, 2020

TLS: improve compliance with shutdown standard, remove hacks #36111

Closed

tniessen mentioned this issue Mar 25, 2021

Node.js Technical Steering Committee (TSC) Meeting 2021-03-25 nodejs/TSC#999

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams #35904

stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams #35904

mscdex commented Oct 31, 2020 •

edited

Loading

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 5, 2020

Mesteery commented Feb 17, 2022

octogonz commented Jul 5, 2022

vtjnash commented Jul 5, 2022

octogonz commented Jul 5, 2022

vtjnash commented Jul 5, 2022

octogonz commented Jul 5, 2022

stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams #35904

stream: regression since v12.16.3 with TLS sockets backed by non-net.Socket streams #35904

Comments

mscdex commented Oct 31, 2020 • edited Loading

What steps will reproduce the bug?

How often does it reproduce? Is there a required condition?

What is the expected behavior?

What do you see instead?

Additional information

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 3, 2020

mmomtchev commented Nov 5, 2020

Mesteery commented Feb 17, 2022

octogonz commented Jul 5, 2022

vtjnash commented Jul 5, 2022

octogonz commented Jul 5, 2022

vtjnash commented Jul 5, 2022

octogonz commented Jul 5, 2022

mscdex commented Oct 31, 2020 •

edited

Loading