http2: discard more frames after GOAWAY

After sending a GOAWAY with NO_ERROR, we should discard all frames
for streams with larger identifiers than the last stream identifier
in the GOAWAY frame. We weren't discarding RST_STREAM frames, which
could cause us to incorrectly detect a protocol error when handling
a RST_STREAM for a discarded stream.

Hoist post-GOAWAY frame discarding higher in the loop rather than
handling it on a per-frame-type basis.

We are also supposed to count discarded DATA frames against
connection-level flow control, possibly sending WINDOW_UPDATE
messages to return the flow control. We weren't doing this;
this is now fixed.

Fixes golang/go#55846

Change-Id: I7603a529c00b8637e648eee9cc4608fb5fa5199b
Reviewed-on: https://go-review.googlesource.com/c/net/+/434909
Reviewed-by: Heschi Kreinick <[email protected]>
Run-TryBot: Damien Neil <[email protected]>
Auto-Submit: Damien Neil <[email protected]>
Reviewed-by: Brad Fitzpatrick <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: LI ZHEN <[email protected]>
Reviewed-by: Antonio Ojea <[email protected]>

Author: neild

http2/server.go

@@ -1459,6 +1459,22 @@ func (sc *serverConn) processFrame(f Frame) error {
 		sc.sawFirstSettings = true
 	}
 
+	// Discard frames for streams initiated after the identified last
+	// stream sent in a GOAWAY, or all frames after sending an error.
+	// We still need to return connection-level flow control for DATA frames.
+	// RFC 9113 Section 6.8.
+	if sc.inGoAway && (sc.goAwayCode != ErrCodeNo || f.Header().StreamID > sc.maxClientStreamID) {
+
+		if f, ok := f.(*DataFrame); ok {
+			if sc.inflow.available() < int32(f.Length) {
+				return sc.countError("data_flow", streamError(f.Header().StreamID, ErrCodeFlowControl))
+			}
+			sc.inflow.take(int32(f.Length))
+			sc.sendWindowUpdate(nil) // conn-level
+		}
+		return nil
+	}
+
 	switch f := f.(type) {
 	case *SettingsFrame:
 		return sc.processSettings(f)
@@ -1501,9 +1517,6 @@ func (sc *serverConn) processPing(f *PingFrame) error {
 		// PROTOCOL_ERROR."
 		return sc.countError("ping_on_stream", ConnectionError(ErrCodeProtocol))
 	}
-	if sc.inGoAway && sc.goAwayCode != ErrCodeNo {
-		return nil
-	}
 	sc.writeFrame(FrameWriteRequest{write: writePingAck{f}})
 	return nil
 }
@@ -1686,16 +1699,6 @@ func (sc *serverConn) processSettingInitialWindowSize(val uint32) error {
 func (sc *serverConn) processData(f *DataFrame) error {
 	sc.serveG.check()
 	id := f.Header().StreamID
-	if sc.inGoAway && (sc.goAwayCode != ErrCodeNo || id > sc.maxClientStreamID) {
-		// Discard all DATA frames if the GOAWAY is due to an
-		// error, or:
-		//
-		// Section 6.8: After sending a GOAWAY frame, the sender
-		// can discard frames for streams initiated by the
-		// receiver with identifiers higher than the identified
-		// last stream.
-		return nil
-	}
 
 	data := f.Data()
 	state, st := sc.state(id)
@@ -1847,10 +1850,6 @@ func (st *stream) onWriteTimeout() {
 func (sc *serverConn) processHeaders(f *MetaHeadersFrame) error {
 	sc.serveG.check()
 	id := f.StreamID
-	if sc.inGoAway {
-		// Ignore.
-		return nil
-	}
 	// http://tools.ietf.org/html/rfc7540#section-5.1.1
 	// Streams initiated by a client MUST use odd-numbered stream
 	// identifiers. [...] An endpoint that receives an unexpected
@@ -2021,9 +2020,6 @@ func (sc *serverConn) checkPriority(streamID uint32, p PriorityParam) error {
 }
 
 func (sc *serverConn) processPriority(f *PriorityFrame) error {
-	if sc.inGoAway {
-		return nil
-	}
 	if err := sc.checkPriority(f.StreamID, f.PriorityParam); err != nil {
 		return err
 	}

http2/server_test.go

@@ -3994,14 +3994,14 @@ func TestServer_Rejects_TooSmall(t *testing.T) {
 // and the connection still completing.
 func TestServerHandlerConnectionClose(t *testing.T) {
 	unblockHandler := make(chan bool, 1)
-	defer close(unblockHandler) // backup; in case of errors
 	testServerResponse(t, func(w http.ResponseWriter, r *http.Request) error {
 		w.Header().Set("Connection", "close")
 		w.Header().Set("Foo", "bar")
 		w.(http.Flusher).Flush()
 		<-unblockHandler
 		return nil
 	}, func(st *serverTester) {
+		defer close(unblockHandler) // backup; in case of errors
 		st.writeHeaders(HeadersFrameParam{
 			StreamID:      1,
 			BlockFragment: st.encodeHeader(),
@@ -4010,6 +4010,7 @@ func TestServerHandlerConnectionClose(t *testing.T) {
 		})
 		var sawGoAway bool
 		var sawRes bool
+		var sawWindowUpdate bool
 		for {
 			f, err := st.readFrame()
 			if err == io.EOF {
@@ -4021,10 +4022,29 @@ func TestServerHandlerConnectionClose(t *testing.T) {
 			switch f := f.(type) {
 			case *GoAwayFrame:
 				sawGoAway = true
-				unblockHandler <- true
 				if f.LastStreamID != 1 || f.ErrCode != ErrCodeNo {
 					t.Errorf("unexpected GOAWAY frame: %v", summarizeFrame(f))
 				}
+				// Create a stream and reset it.
+				// The server should ignore the stream.
+				st.writeHeaders(HeadersFrameParam{
+					StreamID:      3,
+					BlockFragment: st.encodeHeader(),
+					EndStream:     false,
+					EndHeaders:    true,
+				})
+				st.fr.WriteRSTStream(3, ErrCodeCancel)
+				// Create a stream and send data to it.
+				// The server should return flow control, even though it
+				// does not process the stream.
+				st.writeHeaders(HeadersFrameParam{
+					StreamID:      5,
+					BlockFragment: st.encodeHeader(),
+					EndStream:     false,
+					EndHeaders:    true,
+				})
+				// Write enough data to trigger a window update.
+				st.writeData(5, true, make([]byte, 1<<19))
 			case *HeadersFrame:
 				goth := st.decodeHeader(f.HeaderBlockFragment())
 				wanth := [][2]string{
@@ -4039,6 +4059,17 @@ func TestServerHandlerConnectionClose(t *testing.T) {
 				if f.StreamID != 1 || !f.StreamEnded() || len(f.Data()) != 0 {
 					t.Errorf("unexpected DATA frame: %v", summarizeFrame(f))
 				}
+			case *WindowUpdateFrame:
+				if !sawGoAway {
+					t.Errorf("unexpected WINDOW_UPDATE frame: %v", summarizeFrame(f))
+					return
+				}
+				if f.StreamID != 0 {
+					st.t.Fatalf("WindowUpdate StreamID = %d; want 5", f.FrameHeader.StreamID)
+					return
+				}
+				sawWindowUpdate = true
+				unblockHandler <- true
 			default:
 				t.Logf("unexpected frame: %v", summarizeFrame(f))
 			}
@@ -4049,6 +4080,9 @@ func TestServerHandlerConnectionClose(t *testing.T) {
 		if !sawRes {
 			t.Errorf("didn't see response")
 		}
+		if !sawWindowUpdate {
+			t.Errorf("didn't see WINDOW_UPDATE")
+		}
 	})
 }