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: reset stream on response header error #16544

Closed
wants to merge 3 commits into from

Conversation

icing
Copy link
Contributor

@icing icing commented Mar 3, 2025

refs #16535

We send a GOAWAY, but some servers ignore that and happily continue sending the stream response. RST the stream when response header errors are encountered.

@jay
Copy link
Member

jay commented Mar 3, 2025

We send a GOAWAY, but some servers ignore that and happily continue sending the stream response.

Isn't that the way it's supposed to be? The RFC says the GOAWAY applies to streams above the stream identifier used in the GOAWAY.

icing added 2 commits March 4, 2025 11:49
refs curl#16535

We send a GOAWAY, but some servers ignore that and happily continue
sending the stream response. RST the stream when response header
errors are encountered.
When the server sends HEADER/CONTINUATION frames that exceed nghttp2's
size, this error is being reported via the on_invalid_frame_recv
callback. Without registering there, it will go unnoticed.

RST the stream when such a frame is encountered.
@icing
Copy link
Contributor Author

icing commented Mar 4, 2025

We send a GOAWAY, but some servers ignore that and happily continue sending the stream response.

Isn't that the way it's supposed to be? The RFC says the GOAWAY applies to streams above the stream identifier used in the GOAWAY.

Yes, indeed. Nginx seems to disagree.

Am about to add some more in our implementation in this PR to cope with this.

nghttp2 will on its own send GOAWAY frames, closing the connection,
when internal processing of frames runs into errors. This may not
become visible in a direct error code from a call to nghttp2.

Check for session being closed on ingress processing (on sending,
we already did that) and report an error if so. In addition,
monitor outgoing GOAWAY not initiated by us so that the user will
get a fail message when that happens.

Add some more long response header tests.
@icing icing force-pushed the h2-reset-on-header-err branch from ff28c1e to 819733b Compare March 4, 2025 11:34
@github-actions github-actions bot added the tests label Mar 4, 2025
@bagder bagder closed this in e7de83a Mar 5, 2025
bagder pushed a commit that referenced this pull request Mar 5, 2025
When the server sends HEADER/CONTINUATION frames that exceed nghttp2's
size, this error is being reported via the on_invalid_frame_recv
callback. Without registering there, it will go unnoticed.

RST the stream when such a frame is encountered.

Closes #16544
bagder pushed a commit that referenced this pull request Mar 5, 2025
nghttp2 will on its own send GOAWAY frames, closing the connection, when
internal processing of frames runs into errors. This may not become
visible in a direct error code from a call to nghttp2.

Check for session being closed on ingress processing (on sending, we
already did that) and report an error if so. In addition, monitor
outgoing GOAWAY not initiated by us so that the user will get a fail
message when that happens.

Add some more long response header tests.

Closes #16544
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants