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 <[email protected]>

Author: bradfitz

http2/transport.go

@@ -1255,22 +1255,40 @@ func (rl *clientConnReadLoop) processData(f *DataFrame) error {
 	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() {

http2/transport_test.go

@@ -1230,16 +1230,17 @@ func TestTransportChecksResponseHeaderListSize(t *testing.T) {
 		}
 	}
 	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 @@ func TestTransportBodyReadErrorType(t *testing.T) {
 		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}