Skip to content

Commit

Permalink
http2: discard more frames after GOAWAY
Browse files Browse the repository at this point in the history
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]>
  • Loading branch information
WeiminShang authored and gopherbot committed Oct 27, 2022
1 parent d9416fc commit 05c9dea
Show file tree
Hide file tree
Showing 2 changed files with 52 additions and 22 deletions.
36 changes: 16 additions & 20 deletions http2/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
38 changes: 36 additions & 2 deletions http2/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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 {
Expand All @@ -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{
Expand All @@ -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))
}
Expand All @@ -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")
}
})
}

Expand Down

0 comments on commit 05c9dea

Please sign in to comment.