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

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

Open
wants to merge 52 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
52 commits
Select commit Hold shift + click to select a range
4d7aba2
http2, tls: check content-length, fix RST and GOAWAY logic
szmarczak Oct 10, 2024
60bdd9f
Check content-length before calling stream_close_callback with error …
Sep 15, 2020
0ee22c8
fix(http2): check content-length, fix sending RST
szmarczak May 26, 2024
ca3c23d
fix a test
szmarczak May 26, 2024
2cd1b95
[skip ci] fix: lint
szmarczak May 26, 2024
12093cb
[skip ci] fix 1 flaky test, 3 more to go
szmarczak May 26, 2024
c363085
more bug fixes
szmarczak May 26, 2024
771bf72
[skip ci] fix a test
szmarczak May 26, 2024
0ad5ec5
more bug fixes
szmarczak May 26, 2024
3191fb7
fix two tests
szmarczak May 27, 2024
b1905dd
fix goaway bugs
szmarczak May 27, 2024
c2a96a7
fix last test? ill check linux in a sec
szmarczak May 27, 2024
87c36dd
fix a bug
szmarczak May 28, 2024
5eeb849
format cpp
szmarczak May 28, 2024
22d9bb2
fix tests
szmarczak May 29, 2024
a9b456e
NGHTTP2_CONNECT_ERROR
szmarczak May 29, 2024
0c32c92
test econnreset
szmarczak May 29, 2024
e67926d
[skip ci] hascrypto
szmarczak May 29, 2024
2ed1755
bug fixes
szmarczak May 29, 2024
3334e47
fix
szmarczak May 29, 2024
14f6371
fix errors
szmarczak May 29, 2024
de563b1
more verbose error checks
szmarczak May 29, 2024
107dc65
refactor _destroy
szmarczak May 29, 2024
d44fbbb
less diff
szmarczak May 29, 2024
8994699
fix dropped rsts and goaways
szmarczak May 30, 2024
a48b01f
[skip ci] fix missing return
szmarczak May 30, 2024
0872c51
[skip ci] fix that return
szmarczak May 30, 2024
8b75779
throw on frame error
szmarczak May 30, 2024
28c80e5
fix a test
szmarczak May 30, 2024
64a1337
fix frame error
szmarczak May 30, 2024
23f3b8c
fix frame errors
szmarczak May 30, 2024
585dd24
send rst on frame error if server
szmarczak May 30, 2024
68ef526
fix that again
szmarczak May 30, 2024
384b09f
fix that again
szmarczak May 30, 2024
8275e2e
Check if session_call_on_frame_received frees the stream in session_a…
szmarczak Oct 4, 2024
8fa7711
patch
szmarczak Oct 4, 2024
c59704a
Allow RST_STREAM before GOAWAY
szmarczak Oct 4, 2024
1b3ad59
fixup
szmarczak Oct 4, 2024
1d57327
fix
szmarczak Oct 6, 2024
e78ba74
fix
szmarczak Oct 6, 2024
0c3ac35
Fix server close not being emitted if TLS socket RSTs
szmarczak Oct 6, 2024
9f96023
fix
szmarczak Oct 6, 2024
7c152da
does not matter if TCP FIN or TCP RST
szmarczak Oct 8, 2024
4496988
default to NGHTTP2_INTERNAL_ERROR
szmarczak Oct 8, 2024
126fa33
lint
szmarczak Oct 10, 2024
4353b3f
fix flaky test-http2-respond-with-file-connection-abort.js
szmarczak Oct 15, 2024
246b623
fix lint
szmarczak Oct 15, 2024
5ce564d
try to fix windows
szmarczak Oct 15, 2024
b9b3136
fix moot condition
szmarczak Oct 15, 2024
58be6b5
remove moot errorSendingFrame
szmarczak Oct 16, 2024
56dea47
note
szmarczak Oct 16, 2024
2f4d66f
quick fix
szmarczak Oct 16, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benchmark/http2/headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function main({ n, nheaders }) {
} else {
bench.end(n);
server.close();
client.destroy();
client.close();
}
});
}
Expand Down
13 changes: 10 additions & 3 deletions deps/nghttp2/lib/nghttp2_session.c
Original file line number Diff line number Diff line change
Expand Up @@ -1485,6 +1485,11 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id,

DEBUGF("stream: stream(%p)=%d close\n", stream, stream->stream_id);

if (error_code == NGHTTP2_NO_ERROR &&
nghttp2_http_on_remote_end_stream(stream) == -1) {
error_code = NGHTTP2_PROTOCOL_ERROR;
}

/* We call on_stream_close_callback even if stream->state is
NGHTTP2_STREAM_INITIAL. This will happen while sending request
HEADERS, a local endpoint receives RST_STREAM for that stream. It
Expand Down Expand Up @@ -2516,9 +2521,6 @@ static int session_prep_frame(nghttp2_session *session,
return 0;
}
case NGHTTP2_RST_STREAM:
if (session_is_closing(session)) {
return NGHTTP2_ERR_SESSION_CLOSING;
}
nghttp2_frame_pack_rst_stream(&session->aob.framebufs, &frame->rst_stream);
return 0;
case NGHTTP2_SETTINGS: {
Expand Down Expand Up @@ -4181,6 +4183,11 @@ static int session_after_header_block_received(nghttp2_session *session) {
return 0;
}

/* on_frame might free the stream */
if (!nghttp2_session_get_stream(session, frame->hd.stream_id)) {
return 0;
}

return session_end_stream_headers_received(session, frame, stream);
}

Expand Down
29 changes: 29 additions & 0 deletions deps/nghttp2/patches/0000-content-length.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
From 92c120c0ba2ed5806960f8ec9a338fefc17b0427 Mon Sep 17 00:00:00 2001
From: Gil Pedersen <[email protected]>
Date: Tue, 15 Sep 2020 12:36:34 +0000
Subject: [PATCH] Check content-length before calling stream_close_callback
with error code 0

---
deps/nghttp2/lib/nghttp2_session.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c
index 004a4dff..008f9d34 100644
--- a/deps/nghttp2/lib/nghttp2_session.c
+++ b/deps/nghttp2/lib/nghttp2_session.c
@@ -1485,6 +1485,11 @@ int nghttp2_session_close_stream(nghttp2_session *session, int32_t stream_id,

DEBUGF("stream: stream(%p)=%d close\n", stream, stream->stream_id);

+ if (error_code == NGHTTP2_NO_ERROR &&
+ nghttp2_http_on_remote_end_stream(stream) == -1) {
+ error_code = NGHTTP2_PROTOCOL_ERROR;
+ }
+
/* We call on_stream_close_callback even if stream->state is
NGHTTP2_STREAM_INITIAL. This will happen while sending request
HEADERS, a local endpoint receives RST_STREAM for that stream. It
--
2.40.1

Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
From 9581880841f5afa00075eb6089389c03015334ce Mon Sep 17 00:00:00 2001
From: Szymon Marczak <[email protected]>
Date: Fri, 4 Oct 2024 08:08:15 +0200
Subject: [PATCH] Check if session_call_on_frame_received frees the stream in
session_after_header_block_received

---
deps/nghttp2/lib/nghttp2_session.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c
index 08efaf0f8e..8b818c30a2 100644
--- a/deps/nghttp2/lib/nghttp2_session.c
+++ b/deps/nghttp2/lib/nghttp2_session.c
@@ -4186,6 +4186,11 @@ static int session_after_header_block_received(nghttp2_session *session) {
return 0;
}

+ /* on_frame might free the stream */
+ if (!nghttp2_session_get_stream(session, frame->hd.stream_id)) {
+ return 0;
+ }
+
return session_end_stream_headers_received(session, frame, stream);
}

--
2.46.2

26 changes: 26 additions & 0 deletions deps/nghttp2/patches/0002-rst-before-goaway.patch
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
From a990ac8598241f939ca41bf7a7c53c40094663bf Mon Sep 17 00:00:00 2001
From: Szymon Marczak <[email protected]>
Date: Fri, 4 Oct 2024 11:52:30 +0200
Subject: [PATCH] Allow RST_STREAM before GOAWAY

---
deps/nghttp2/lib/nghttp2_session.c | 3 ---
1 file changed, 3 deletions(-)

diff --git a/deps/nghttp2/lib/nghttp2_session.c b/deps/nghttp2/lib/nghttp2_session.c
index 8b818c30a2..67d8b08d36 100644
--- a/deps/nghttp2/lib/nghttp2_session.c
+++ b/deps/nghttp2/lib/nghttp2_session.c
@@ -2521,9 +2521,6 @@ static int session_prep_frame(nghttp2_session *session,
return 0;
}
case NGHTTP2_RST_STREAM:
- if (session_is_closing(session)) {
- return NGHTTP2_ERR_SESSION_CLOSING;
- }
nghttp2_frame_pack_rst_stream(&session->aob.framebufs, &frame->rst_stream);
return 0;
case NGHTTP2_SETTINGS: {
--
2.46.2

2 changes: 1 addition & 1 deletion doc/api/http2.md
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ added: v8.4.0
due to an error.
* `code` {number} The HTTP/2 error code to send in the final `GOAWAY` frame.
If unspecified, and `error` is not undefined, the default is `INTERNAL_ERROR`,
otherwise defaults to `NO_ERROR`.
otherwise defaults to `CANCEL`.

Immediately terminates the `Http2Session` and the associated `net.Socket` or
`tls.TLSSocket`.
Expand Down
4 changes: 4 additions & 0 deletions lib/_tls_wrap.js
Original file line number Diff line number Diff line change
Expand Up @@ -810,6 +810,10 @@ TLSSocket.prototype._init = function(socket, wrap) {
const ssl = this._handle;
this.server = options.server;

// Required for Socket.prototype._destroy check
// to emit 'close' server event
this._server = options.server;

debug('%s _init',
options.isServer ? 'server' : 'client',
'handle?', !!ssl,
Expand Down
Loading
Loading