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.

Sign up for GitHub

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

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

Show file tree
Hide file tree
Changes from 1 commit
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 …
kanongil 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
4 changes: 1 addition & 3 deletions lib/internal/http2/core.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -695,8 +695,6 @@ function onGoawayData(code, lastStreamID, buf) {
session[kState].flags |= SESSION_FLAGS_CLOSED;
session[kMaybeDestroy]();
} else {
session[kState].flags |= SESSION_FLAGS_DESTROYED;

// Must not send any further data.
closeSession(
session,
Expand Down Expand Up @@ -3326,7 +3324,7 @@ function socketOnClose() {

state.streams.forEach(closeStream);
state.pendingStreams.forEach(closeStream);
session.close();
Copy link
Member Author

@szmarczak szmarczak Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed it fixed that!

session[kState].flags |= SESSION_FLAGS_CLOSED;
session[kMaybeDestroy](err);
}
}
Expand Down
12 changes: 4 additions & 8 deletions test/parallel/test-http2-server-sessionerror.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,19 +45,15 @@ server.listen(0, common.mustCall(() => {
const url = `http://localhost:${server.address().port}`;
http2.connect(url)
.on('error', common.mustCall((err) => {
if (err.code !== 'ECONNRESET') {
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR');
}
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR');
}))
.on('close', () => {
server.removeAllListeners('error');
http2.connect(url)
.on('error', common.mustCall((err) => {
if (err.code !== 'ECONNRESET') {
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR');
}
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR');
}))
.on('close', () => server.close());
});
Expand Down
6 changes: 2 additions & 4 deletions test/parallel/test-http2-too-many-settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,8 @@ function doTest(session) {
server.listen(0, common.mustCall(() => {
const client = h2.connect(`http://localhost:${server.address().port}`);
client.on('error', common.mustCall((err) => {
if (err.code !== 'ECONNRESET') {
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR');
}
assert.strictEqual(err.code, 'ERR_HTTP2_SESSION_ERROR');
assert.strictEqual(err.message, 'Session closed with error code NGHTTP2_INTERNAL_ERROR');
}));
client.on('close', common.mustCall(() => server.close()));
}));
Expand Down
Loading