http2: add Transport strictness, paranoia, logging for unwanted DATA frames
I can't reproduce the user's bug yet, but this might fix or at least
help clarify what's happening.
Also deflakes a test.
Updates golang/go#13932
Change-Id: If56bdd833f183d4502701e65e56749434bd82150
Reviewed-on: https://go-review.googlesource.com/18576
Reviewed-by: Andrew Gerrand <adg@golang.org>
diff --git a/http2/transport.go b/http2/transport.go
index 4ae993b..5be1991 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -1255,22 +1255,40 @@
cc := rl.cc
cs := cc.streamByID(f.StreamID, f.StreamEnded())
if cs == nil {
+ cc.mu.Lock()
+ neverSent := cc.nextStreamID
+ cc.mu.Unlock()
+ if f.StreamID >= neverSent {
+ // We never asked for this.
+ cc.logf("http2: Transport received unsolicited DATA frame; closing connection")
+ return ConnectionError(ErrCodeProtocol)
+ }
+ // We probably did ask for this, but canceled. Just ignore it.
+ // TODO: be stricter here? only silently ignore things which
+ // we canceled, but not things which were closed normally
+ // by the peer? Tough without accumulating too much state.
return nil
}
- data := f.Data()
+ if data := f.Data(); len(data) > 0 {
+ if cs.bufPipe.b == nil {
+ // Data frame after it's already closed?
+ cc.logf("http2: Transport received DATA frame for closed stream; closing connection")
+ return ConnectionError(ErrCodeProtocol)
+ }
- // Check connection-level flow control.
- cc.mu.Lock()
- if cs.inflow.available() >= int32(len(data)) {
- cs.inflow.take(int32(len(data)))
- } else {
+ // Check connection-level flow control.
+ cc.mu.Lock()
+ if cs.inflow.available() >= int32(len(data)) {
+ cs.inflow.take(int32(len(data)))
+ } else {
+ cc.mu.Unlock()
+ return ConnectionError(ErrCodeFlowControl)
+ }
cc.mu.Unlock()
- return ConnectionError(ErrCodeFlowControl)
- }
- cc.mu.Unlock()
- if _, err := cs.bufPipe.Write(data); err != nil {
- return err
+ if _, err := cs.bufPipe.Write(data); err != nil {
+ return err
+ }
}
if f.StreamEnded() {
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 4a42f66..cd6f3df 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -1230,16 +1230,17 @@
}
}
ct.run()
-
}
// Test that the the Transport returns a typed error from Response.Body.Read calls
// when the server sends an error. (here we use a panic, since that should generate
// a stream error, but others like cancel should be similar)
func TestTransportBodyReadErrorType(t *testing.T) {
+ doPanic := make(chan bool, 1)
st := newServerTester(t,
func(w http.ResponseWriter, r *http.Request) {
w.(http.Flusher).Flush() // force headers out
+ <-doPanic
panic("boom")
},
optOnlyServer,
@@ -1256,6 +1257,7 @@
t.Fatal(err)
}
defer res.Body.Close()
+ doPanic <- true
buf := make([]byte, 100)
n, err := res.Body.Read(buf)
want := StreamError{StreamID: 0x1, Code: 0x2}
