[internal-branch.go1.22-vendor] http2: send correct LastStreamID in s…

…tream-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.

Merge conflicts were avoided by cherry-picking CL 576235 (test deflake)
prior to this, and then by squashing CL 576175 (typo fix) into this CL.

For golang/go#66668.
For golang/go#66698.

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]>
Reviewed-on: https://go-review.googlesource.com/c/net/+/578338
Reviewed-by: Than McIntosh <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>

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 {
@@ -1522,7 +1525,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,8 +1595,8 @@ func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) {
  log.Printf("http2: header list too large")
  }
  // It would be nice to send a RST_STREAM before sending the GOAWAY,
- // but the struture of the server's frame writer makes this difficult.
- return nil, ConnectionError(ErrCodeProtocol)
+ // but the structure of the server's frame writer makes this difficult.
+ return mh, ConnectionError(ErrCodeProtocol)
  }
 
  // Also close the connection after any CONTINUATION frame following an
@@ -1604,12 +1607,12 @@ func (fr *Framer) readMetaFrame(hf *HeadersFrame) (*MetaHeadersFrame, error) {
  log.Printf("http2: invalid header: %v", invalid)
  }
  // It would be nice to send a RST_STREAM before sending the GOAWAY,
- // but the struture of the server's frame writer makes this difficult.
- return nil, ConnectionError(ErrCodeProtocol)
+ // but the structure of the server's frame writer makes this difficult.
+ return mh, ConnectionError(ErrCodeProtocol)
  }
 
  if _, err := hdec.Write(frag); err != nil {
- return nil, ConnectionError(ErrCodeCompression)
+ return mh, ConnectionError(ErrCodeCompression)
  }
 
  if hc.HeadersEnded() {
@@ -1626,7 +1629,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

@@ -1481,6 +1481,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

@@ -4809,9 +4809,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,