fix: reject GOAWAY frames with a non-zero stream identifier - #951
Merged
Conversation
Problem RFC 9113 section 6.8 says an endpoint MUST treat a GOAWAY frame with a stream identifier other than 0x00 as a connection error of type PROTOCOL_ERROR. `GoAway::load` never saw the frame head, so the stream identifier was not checked and such a frame was accepted as an ordinary connection shutdown. Solution Pass the head into `GoAway::load` and reject a non-zero stream id, the same way SETTINGS and PING already do. Validation New codec test `read_goaway_stream_id_not_zero` fails before the change and passes after. Against the example server, a GOAWAY on stream 1 used to get GOAWAY(NO_ERROR) back and now gets GOAWAY(PROTOCOL_ERROR); a GOAWAY on stream 0 is unchanged. h2spec 2.6 --strict is unchanged at 146 passed / 1 skipped / 0 failed.
jaideeppyne
force-pushed
the
goaway-stream-id
branch
from
August 31, 2026 13:17
5fbab78 to
a8a42de
Compare
seanmonstar
approved these changes
Aug 31, 2026
seanmonstar
left a comment
seanmonstar
left a comment
Member
There was a problem hiding this comment.
Nice improvement, thanks!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
RFC 9113 section 6.8:
GoAway::loadis the only frame loader that never got the frame head, so the stream id was never looked at. A GOAWAY sent on stream 1 was accepted as a normal connection shutdown.I found this probing the connection-level frames against the spec. SETTINGS, PING and RST_STREAM all already reject a bad stream id; GOAWAY was the odd one out. Against
examples/server, GOAWAY on stream 1 gotGOAWAY(NO_ERROR)back, byte for byte the same as the legal stream-0 case. With this change it getsGOAWAY(PROTOCOL_ERROR), and stream 0 still getsNO_ERROR.The fix passes
headintoloadand checksis_zero(), matching whatPing::loaddoes. The existing generic error mapping inframed_readalready turns that into a connection-level PROTOCOL_ERROR.Oracle and counts:
read_goaway_stream_id_not_zerofails before, passes after (I reverted only the two source files and kept the test to confirm:expected error; actual=Some(Ok(GoAway { error_code: NO_ERROR, .. })))--strictagainstexamples/serveris unchanged: 147 tests, 146 passed, 1 skipped, 0 failed. h2spec's 6.8 test passes either way because it accepts a closed connection as evidence of a connection error, and h2 was closing anyway by honouring the GOAWAYcargo test --all-featuresis unchangedUnrelated, but
proto::streams::recv::tests::clear_recv_buffer_caps_capacity_before_overflowalready fails on a pristine master for me (assertion failed: self.slab.is_empty()at store.rs:231). Not touched here, happy to file it separately.AI disclosure: I used Claude Code to help with this change.