http2, tls: check content-length, fix RST and GOAWAY logic #53160

szmarczak · 2024-05-26T05:36:49Z

Time spent: 90h

Caution

These bug fixes are potentially semver-major!

Fixed stream.close(NGHTTP2_CANCEL) closing with 0 aka NGHTTP2_NO_ERROR.
serverStream.destroy() closed with 0, now NGHTTP2_INTERNAL_ERROR.
clientStream.destroy() closed with 0, now NGHTTP2_CANCEL.
Mismatched content-length now throws NGHTTP2_PROTOCOL_ERROR as according to the spec.
Fixed GOAWAY (server -> client) being greeted with GOAWAY (client -> server) as it's against the spec.
Fixed streams being always closed with NGHTTP2_CANCEL on socket close, now respects goawayCode and destroyCode. The default is NGHTTP2_INTERNAL_ERROR for premature TCP FIN and TCP RST.
Fixed stream.rstCode being 0 while active - docs say it should be undefined.
Fixed preferring sessionCode over rstCode when destroying a stream with an error.
Fixed streams being closed with NO_ERROR if session received GOAWAY with NO_ERROR and remote closed connection.
Fixed streams being closed with NO_ERROR if session sent GOAWAY with NO_ERROR and destroyed.
Fixed GOAWAY preventing RST_STREAM from being sent before GOAWAY.
nghttp2 correctly assumes that it should prevent RST_STREAM from being sent but incorretly applies it to all frames in a packet instead of frames defined after GOAWAY. This is not the only thing that nghttp2 does wrong:
- GOAWAY is sent instead of stream RST when receiving DATA frame after receiving RST_STREAM nghttp2/nghttp2#1166
- nghttp2_submit_rst_stream after nghttp2_submit_response causes response headers to be dropped nghttp2/nghttp2#692
Fixed benchmark/http2/headers.js calling client.destroy() (resulting in dropped requests).
Now calls client.close() which closes gracefully.
connectStreamSocket.destroy() now closes with NGHTTP2_CONNECT_ERROR.
Fixed double GOAWAY on session.close().
Fixed queued RST_STREAM and GOAWAY being dropped if Http2Session::Close() and previous writes weren't finished yet.
Fixed stream frame errors closing the entire session instead of just the stream.
Fixed stream frame errors sending RST_STREAM on idle streams (to reproduce 16. needs to be fixed).
Fixed resetAndDestroy can not be called on tlssocket? #49484
Fixed server close not being emitted if TLS socket RSTs
Fixed deadlock when sending GOAWAY before receiving server's GOAWAY. It won't gracefully close because the server may never ACK our GOAWAY. Now it sends TCP RST after terminating GOAWAY.

Sorry so many bugs are fixed in a single PR but it's impossible to fix one without fixing them all!

Best if nghttp2/nghttp2#1508 got merged before this, but it has been open for years 😢
Hence the patch for nghttp2_session.c

/cc @jasnell

nodejs-github-bot · 2024-05-26T05:36:54Z

Review requested:

@nodejs/http
@nodejs/http2
@nodejs/net
@nodejs/security-wg

mcollina

Wow. congrats! It would really be better if the patch was landed, yes.

mcollina · 2024-05-26T07:20:38Z

The linter and commit validator are failing, could you check?

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

mcollina · 2024-05-29T12:45:56Z

@jasnell could you reach out to the nghttp2 maintainers and ask about the status of that PR? I would sincerely hope we wouldn't have to maintain a floating patch.

mcollina

Therefore the best change - although breaking - is to make it emit an error by default.

The reason why .destroy() does not emit an error by default is that it's a Stream API. .destroy() without error is supposed to be "safe" to call.

nodejs-github-bot · 2024-05-29T12:52:33Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59515/

szmarczak · 2024-05-29T14:42:00Z

test/parallel/test-http2-client-rststream-before-connect.js

-  req.on('error', common.expectsError({
-    code: 'ERR_HTTP2_STREAM_ERROR',
-    name: 'Error',
-    message: 'Stream closed with error code NGHTTP2_PROTOCOL_ERROR'
-  }));
-


I don't think this should be removed

@mcollina This is a result of the fix for making .destroy() not throw when sending RST. It throws only when receiving RST. This means that stream.close(1) won't emit error. Are you okay with this change? By looking at the original code for close() it looks like the author didn't mean to emit error on close().

~~Otherwise I would have to make stream.close() set stream[kState].rstCode and then call this.destroy(error).~~

Actually my understanding was wrong. It shouldn't be removed indeed.

It should error via onStreamClose (so pipeline fails etc.) but fails to do so because

node/lib/internal/http2/core.js

Line 2400 in 56dea47

this.destroy();

gets called first.

I'm not fluent in stream internals but setImmediate seems to work here (in afterShutdown), so I went with that.

Looks like the main cause is that writable shutdown is delayed by single tick in order to attach the END_STREAM flag. It's a pity nghttp2 has no API to amend the last queued DATA frame.

test/parallel/test-http2-client-socket-destroy.js

test/parallel/test-http2-server-shutdown-redundant.js

mcollina · 2024-05-30T11:47:31Z

Have you tried pinging nghttp2?

mcollina

lgtm

nodejs-github-bot · 2024-05-30T13:16:54Z

CI: https://ci.nodejs.org/job/node-test-pull-request/59539/

szmarczak · 2024-10-10T22:40:07Z

@mcollina Can you trigger CI again? Thanks 🙏

The lint errors and test failures on GitHub Actions seem to be unrelated to this PR.

nodejs-github-bot · 2024-10-12T05:22:08Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63062/

szmarczak · 2024-10-15T18:45:10Z

lib/internal/http2/core.js

@@ -3326,7 +3324,7 @@ function socketOnClose() {

    state.streams.forEach(closeStream);
    state.pendingStreams.forEach(closeStream);
-    session.close();


session.close() (with this PR) calls session.destroy() if state.streams and state.pendingStreams are empty. Therefore it could obstruct session[kMaybeDestroy](err). Now it just marks the session as closed, hopefully fixing Windows: test-http2-server-sessionerror.js and test-http2-too-many-settings.js that do not create any streams.

Indeed it fixed that!

szmarczak · 2024-10-15T18:45:59Z

@mcollina Can you trigger CI again? Hopefully Windows passes this time 🙏🏼

nodejs-github-bot · 2024-10-17T14:58:02Z

CI: https://ci.nodejs.org/job/node-test-pull-request/63162/

szmarczak · 2024-10-17T19:56:29Z

Windows passes now.

test-http2-large-file is flaky and I couldn't reproduce:

test-http-server-request-timeouts-mixed is also flaky.

test-http2-session-unref fails on aix. Why do we even support aix? I can't even work on this because it's so exclusive.

IMO this PR is good to go. I'd prefer if this landed first in v23, then backport to v22 if necessary.

mcollina · 2024-10-21T13:34:32Z

test-http2-session-unref fails on aix. Why do we even support aix? I can't even work on this because it's so exclusive.

@mhdawson can you help with this?

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 26, 2024

mcollina approved these changes May 26, 2024

View reviewed changes

szmarczak commented May 29, 2024

View reviewed changes

test/parallel/test-http2-client-connection-tunnelling.js Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

mcollina added request-ci Add this label to start a Jenkins CI on a PR. baking-for-lts PRs that need to wait before landing in a LTS release. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. labels May 29, 2024

mcollina requested a review from jasnell May 29, 2024 12:45

mcollina requested changes May 29, 2024

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 29, 2024

szmarczak commented May 29, 2024

View reviewed changes

test/parallel/test-http2-client-socket-destroy.js Show resolved Hide resolved

szmarczak commented May 29, 2024

View reviewed changes

test/parallel/test-http2-server-shutdown-redundant.js Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

This comment was marked as outdated.

Sign in to view

This comment was marked as resolved.

Sign in to view

mcollina approved these changes May 30, 2024

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 30, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 30, 2024

This comment was marked as resolved.

Sign in to view

This comment was marked as off-topic.

Sign in to view

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2024

szmarczak added 10 commits October 10, 2024 23:03

patch

8fa7711

Allow RST_STREAM before GOAWAY

c59704a

fixup

1b3ad59

fix

1d57327

fix

e78ba74

Fix server close not being emitted if TLS socket RSTs

0c3ac35

fix

9f96023

does not matter if TCP FIN or TCP RST

7c152da

default to NGHTTP2_INTERNAL_ERROR

4496988

lint

126fa33

szmarczak force-pushed the fix-http2 branch from c9eb300 to 126fa33 Compare October 10, 2024 21:04

szmarczak changed the title ~~fix(http2): check content-length, fix sending RST~~ http2, tls: check content-length, fix RST and GOAWAY logic Oct 10, 2024

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 12, 2024

szmarczak added 3 commits October 15, 2024 16:22

fix flaky test-http2-respond-with-file-connection-abort.js

4353b3f

fix lint

246b623

try to fix windows

5ce564d

szmarczak commented Oct 15, 2024

View reviewed changes

szmarczak added 4 commits October 16, 2024 01:56

fix moot condition

b9b3136

remove moot errorSendingFrame

58be6b5

note

56dea47

quick fix

2f4d66f

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 17, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http2, tls: check content-length, fix RST and GOAWAY logic #53160

http2, tls: check content-length, fix RST and GOAWAY logic #53160

szmarczak commented May 26, 2024 •

edited

Loading

nodejs-github-bot commented May 26, 2024

mcollina left a comment

mcollina commented May 26, 2024

This comment was marked as resolved.

mcollina commented May 29, 2024

mcollina left a comment

nodejs-github-bot commented May 29, 2024

szmarczak May 29, 2024 •

edited

Loading

szmarczak May 29, 2024 •

edited

Loading

szmarczak Oct 16, 2024

szmarczak Oct 16, 2024

This comment was marked as resolved.

This comment was marked as outdated.

mcollina commented May 30, 2024

This comment was marked as resolved.

mcollina left a comment

nodejs-github-bot commented May 30, 2024

This comment was marked as resolved.

This comment was marked as off-topic.

szmarczak commented Oct 10, 2024

nodejs-github-bot commented Oct 12, 2024

szmarczak Oct 15, 2024 •

edited

Loading

szmarczak Oct 17, 2024

szmarczak commented Oct 15, 2024

nodejs-github-bot commented Oct 17, 2024

szmarczak commented Oct 17, 2024

mcollina commented Oct 21, 2024

http2, tls: check content-length, fix RST and GOAWAY logic #53160

Are you sure you want to change the base?

http2, tls: check content-length, fix RST and GOAWAY logic #53160

Conversation

szmarczak commented May 26, 2024 • edited Loading

nodejs-github-bot commented May 26, 2024

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented May 26, 2024

This comment was marked as resolved.

mcollina commented May 29, 2024

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 29, 2024

szmarczak May 29, 2024 • edited Loading

Choose a reason for hiding this comment

szmarczak May 29, 2024 • edited Loading

Choose a reason for hiding this comment

szmarczak Oct 16, 2024

Choose a reason for hiding this comment

szmarczak Oct 16, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as outdated.

mcollina commented May 30, 2024

This comment was marked as resolved.

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented May 30, 2024

This comment was marked as resolved.

This comment was marked as off-topic.

szmarczak commented Oct 10, 2024

nodejs-github-bot commented Oct 12, 2024

szmarczak Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

szmarczak Oct 17, 2024

Choose a reason for hiding this comment

szmarczak commented Oct 15, 2024

nodejs-github-bot commented Oct 17, 2024

szmarczak commented Oct 17, 2024

mcollina commented Oct 21, 2024

szmarczak commented May 26, 2024 •

edited

Loading

szmarczak May 29, 2024 •

edited

Loading

szmarczak May 29, 2024 •

edited

Loading

szmarczak Oct 15, 2024 •

edited

Loading