From b67a0f0535536b3787161ec42c08549b686a5629 Mon Sep 17 00:00:00 2001 From: Damien Neil Date: Thu, 4 Apr 2024 18:18:37 -0700 Subject: [PATCH] http2: send correct LastStreamID in stream-caused GOAWAY When closing a connection because a stream contained a request we didn't like (for example, because the request headers exceed the maximum we will accept), set the LastStreamID in the GOAWAY frame to include the offending stream. This informs the client that retrying the request is unlikely to succeed, and avoids retry loops. This change requires passing the stream ID of the offending stream from Framer.ReadFrame up to the caller. The most sensible way to do this would probably be in the error. However, ReadFrame currently returns a defined error type for connection-ending errors (ConnectionError), and that type is a uint32 with no place to put the stream ID. Rather than changing the returned errors, ReadFrame now returns an error along with a non-nil Frame containing the stream ID, when a stream is responsible for a connection-ending error. For golang/go#66668 Change-Id: Iba07ccbd70ab4939aa56903605474d01703ac6e4 Reviewed-on: https://go-review.googlesource.com/c/net/+/576756 Reviewed-by: Jonathan Amsterdam Reviewed-by: Dmitri Shuralyov Auto-Submit: Damien Neil LUCI-TryBot-Result: Go LUCI --- http2/frame.go | 13 ++++++++----- http2/server.go | 5 +++++ http2/server_test.go | 10 +++++++++- 3 files changed, 22 insertions(+), 6 deletions(-) diff --git a/http2/frame.go b/http2/frame.go index 43557ab7e..105c3b279 100644 --- a/http2/frame.go +++ b/http2/frame.go @@ -490,6 +490,9 @@ func terminalReadFrameError(err error) bool { // returned error is ErrFrameTooLarge. Other errors may be of type // ConnectionError, StreamError, or anything else from the underlying // reader. +// +// If ReadFrame returns an error and a non-nil Frame, the Frame's StreamID +// indicates the stream responsible for the error. func (fr *Framer) ReadFrame() (Frame, error) { fr.errDetail = nil if fr.lastFrame != nil { @@ -1521,7 +1524,7 @@ func (fr *Framer) maxHeaderStringLen() int { // readMetaFrame returns 0 or more CONTINUATION frames from fr and // merge them into the provided hf and returns a MetaHeadersFrame // with the decoded hpack values. -func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) { +func (fr *Framer) readMetaFrame(hf *HeadersFrame) (Frame, error) { if fr.AllowIllegalReads { return nil, errors.New("illegal use of AllowIllegalReads with ReadMetaHeaders") } @@ -1592,7 +1595,7 @@ func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) { } // It would be nice to send a RST_STREAM before sending the GOAWAY, // but the structure of the server's frame writer makes this difficult. - return nil, ConnectionError(ErrCodeProtocol) + return mh, ConnectionError(ErrCodeProtocol) } // Also close the connection after any CONTINUATION frame following an @@ -1604,11 +1607,11 @@ func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) { } // It would be nice to send a RST_STREAM before sending the GOAWAY, // but the structure of the server's frame writer makes this difficult. - return nil, ConnectionError(ErrCodeProtocol) + return mh, ConnectionError(ErrCodeProtocol) } if _, err := hdec.Write(frag); err != nil { - return nil, ConnectionError(ErrCodeCompression) + return mh, ConnectionError(ErrCodeCompression) } if hc.HeadersEnded() { @@ -1625,7 +1628,7 @@ func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) { mh.HeadersFrame.invalidate() if err := hdec.Close(); err != nil { - return nil, ConnectionError(ErrCodeCompression) + return mh, ConnectionError(ErrCodeCompression) } if invalid != nil { fr.errDetail = invalid diff --git a/http2/server.go b/http2/server.go index ce2e8b40e..278add72b 100644 --- a/http2/server.go +++ b/http2/server.go @@ -1482,6 +1482,11 @@ func (sc *serverConn) processFrameFromReader(res readFrameResult) bool { sc.goAway(ErrCodeFlowControl) return true case ConnectionError: + if res.f != nil { + if id := res.f.Header().StreamID; id > sc.maxClientStreamID { + sc.maxClientStreamID = id + } + } sc.logf("http2: server connection error from %v: %v", sc.conn.RemoteAddr(), ev) sc.goAway(ErrCode(ev)) return true // goAway will handle shutdown diff --git a/http2/server_test.go b/http2/server_test.go index a931a06e5..61c773a54 100644 --- a/http2/server_test.go +++ b/http2/server_test.go @@ -4818,9 +4818,17 @@ func TestServerContinuationFlood(t *testing.T) { if err != nil { break } - switch f.(type) { + switch f := f.(type) { case *HeadersFrame: t.Fatalf("received HEADERS frame; want GOAWAY and a closed connection") + case *GoAwayFrame: + // We might not see the GOAWAY (see below), but if we do it should + // indicate that the server processed this request so the client doesn't + // attempt to retry it. + if got, want := f.LastStreamID, uint32(1); got != want { + t.Errorf("received GOAWAY with LastStreamId %v, want %v", got, want) + } + } } // We expect to have seen a GOAWAY before the connection closes,