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 <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>

Author: neild

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

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

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,