Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[v12.x] tls: reset secureConnecting on client socket #34859

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

PR-URL: #33209
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: Sam Roberts vieuxtech@gmail.com

@nodejs-github-bot nodejs-github-bot added tls Issues and PRs related to the tls subsystem. v12.x labels Aug 20, 2020
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 21, 2020
@nodejs-github-bot
Copy link
Collaborator

@szmarczak
Copy link
Member

Friendly ping :) Can this be addressed quick please? Or is there some extra work to do?

secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

PR-URL: nodejs#33209
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2020
@addaleax
Copy link
Member Author

@szmarczak CI wasn’t passing here, but failing with what looked like related failures. I don’t necessarily have the time to look into those, so feel free to take over :) I’ve rebased and kick off another CI, let’s see how that goes.

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 25, 2020
@nodejs-github-bot
Copy link
Collaborator

@lpinca
Copy link
Member

lpinca commented Oct 3, 2020

It seems the session is destroyed before calling session.close() but I don't why.

diff --git a/test/parallel/test-http2-connect.js b/test/parallel/test-http2-connect.js
index 9ee2e4347f..7267956222 100644
--- a/test/parallel/test-http2-connect.js
+++ b/test/parallel/test-http2-connect.js
@@ -90,7 +90,7 @@ const { connect: tlsConnect } = require('tls');
     };
 
     const onSessionConnect = (session) => {
-      session.close();
+      // session.close();
       server.close();
     };

The error is

events.js:291
      throw er; // Unhandled 'error' event
      ^

Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:766:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1020:10)
    at processTicksAndRejections (internal/process/task_queues.js:84:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}

I wonder if this depends on another backport. There is a comment above Http2Session#onSessionInternalError() which says

// When an error occurs internally at the binding level, immediately
// destroy the session.

@lpinca
Copy link
Member

lpinca commented Oct 3, 2020

The same error (Error [ERR_HTTP2_ERROR]: Protocol error) also happens on master but for some reasons (probably due to stream.destroy() changes) it is swallowed on master if session.close() is called.

I think we can work around the issue by adding a noop listener for the 'error' event or fixing the test so that no Protocol error is emitted.

cc: @davedoesdev

@lpinca
Copy link
Member

lpinca commented Oct 3, 2020

I think the simplest way to fix the issue is

diff --git a/test/parallel/test-http2-connect.js b/test/parallel/test-http2-connect.js
index 9ee2e4347f..18cf07b041 100644
--- a/test/parallel/test-http2-connect.js
+++ b/test/parallel/test-http2-connect.js
@@ -96,7 +96,8 @@ const { connect: tlsConnect } = require('tls');
 
     const clientOptions = {
       port,
-      rejectUnauthorized: false
+      rejectUnauthorized: false,
+      ALPNProtocols: ['h2']
     };
     const socket = tlsConnect(clientOptions, mustCall(onSocketConnect));
   }));

@davedoesdev
Copy link
Contributor

@lpinca seems a reasonable workaround.
What's the cause of the protocol error? The connection is closed before the handshakes complete?

@lpinca
Copy link
Member

lpinca commented Oct 4, 2020

The TLS handshake completes, but without the ALPNProtocols option the Http2Session emits that protocol error.

@davedoesdev
Copy link
Contributor

davedoesdev commented Oct 4, 2020

The TLS handshake completes, but without the ALPNProtocols option the Http2Session emits that protocol error.

Might be some state not updated in the HTTP/2 library. Out of interest, does the issue still occur with

    const onSessionConnect = (session) => process.nextTick(() => {
      session.close();
      server.close();
    });

(or setImmediate)

@lpinca
Copy link
Member

lpinca commented Oct 4, 2020

@davedoesdev yes the test fails with a read ECONNRESET error emitted on the TLSSocket with both process.nextTick() and setImmediate() because it tries to write to a destroyed socket.

lpinca added a commit to lpinca/node that referenced this pull request Oct 9, 2020
Without this, the session is destroyed with the following error

```
Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:756:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1010:10)
    at internal/http2/core.js:1048:7
    at finish (internal/streams/writable.js:731:5)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}
```

The test then calls `session.close()` which tries to write to a
destroyed socket. As a result, an unhandled `ECONNRESET` error is
emitted in the v12 release line.

PR-URL: nodejs#35482
Refs: nodejs#34859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Without this, the session is destroyed with the following error

```
Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:756:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1010:10)
    at internal/http2/core.js:1048:7
    at finish (internal/streams/writable.js:731:5)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}
```

The test then calls `session.close()` which tries to write to a
destroyed socket. As a result, an unhandled `ECONNRESET` error is
emitted in the v12 release line.

PR-URL: nodejs#35482
Refs: nodejs#34859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 9, 2020
@nodejs-github-bot
Copy link
Collaborator

BethGriggs pushed a commit that referenced this pull request Oct 14, 2020
Without this, the session is destroyed with the following error

```
Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:756:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1010:10)
    at internal/http2/core.js:1048:7
    at finish (internal/streams/writable.js:731:5)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}
```

The test then calls `session.close()` which tries to write to a
destroyed socket. As a result, an unhandled `ECONNRESET` error is
emitted in the v12 release line.

PR-URL: #35482
Refs: #34859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

Backport-PR-URL: #34859
PR-URL: #33209
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 3, 2020
Without this, the session is destroyed with the following error

```
Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:756:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1010:10)
    at internal/http2/core.js:1048:7
    at finish (internal/streams/writable.js:731:5)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}
```

The test then calls `session.close()` which tries to write to a
destroyed socket. As a result, an unhandled `ECONNRESET` error is
emitted in the v12 release line.

Backport-PR-URL: #34859
PR-URL: #35482
Refs: #34859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 3b55adc...9ab53b4

@MylesBorins MylesBorins closed this Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
secureConnecting is never set to false on client TLS sockets.
So if Http2Session constructor (in lib/internal/http2/core.js) is
called after secureConnect is emitted, then it will wrongly wait
for a secureConnect event.

This fix sets secureConnecting to false when a client TLS socket
has connected.

Backport-PR-URL: #34859
PR-URL: #33209
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
Without this, the session is destroyed with the following error

```
Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:756:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1010:10)
    at internal/http2/core.js:1048:7
    at finish (internal/streams/writable.js:731:5)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}
```

The test then calls `session.close()` which tries to write to a
destroyed socket. As a result, an unhandled `ECONNRESET` error is
emitted in the v12 release line.

Backport-PR-URL: #34859
PR-URL: #35482
Refs: #34859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021
Without this, the session is destroyed with the following error

```
Error [ERR_HTTP2_ERROR]: Protocol error
    at Http2Session.onSessionInternalError (internal/http2/core.js:756:26)
Emitted 'error' event on ClientHttp2Session instance at:
    at emitClose (internal/http2/core.js:1010:10)
    at internal/http2/core.js:1048:7
    at finish (internal/streams/writable.js:731:5)
    at processTicksAndRejections (internal/process/task_queues.js:80:21) {
  code: 'ERR_HTTP2_ERROR',
  errno: -505
}
```

The test then calls `session.close()` which tries to write to a
destroyed socket. As a result, an unhandled `ECONNRESET` error is
emitted in the v12 release line.

PR-URL: nodejs#35482
Refs: nodejs#34859
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants