http2: fix state tracking for pushed streams

Fix sc.state, which was returning "idle" instead of "closed" if the max
pushed stream ID happened to exceed the max client stream ID. This caused
us to erroneously generate connection errors because we believed a state
invariant had been violated when it had not.

I also renamed maxStreamID to maxClientStreamID for clarity, since we
need to track client-generated and server-generated stream IDs separately.

Fixes golang/go#17777

Change-Id: Id3d5700d79cc699a267bd91d6ebace0770fa62fc
Reviewed-on: https://go-review.googlesource.com/32755
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

http2/server.go

@@ -386,8 +386,8 @@ type serverConn struct {
 	advMaxStreams         uint32 // our SETTINGS_MAX_CONCURRENT_STREAMS advertised the client
 	curClientStreams      uint32 // number of open streams initiated by the client
 	curPushedStreams      uint32 // number of open streams initiated by server push
-	maxStreamID           uint32 // max ever seen from client
-	maxPushPromiseID      uint32 // ID of the last push promise, or 0 if there have been no pushes
+	maxClientStreamID     uint32 // max ever seen from client (odd), or 0 if there have been no client requests
+	maxPushPromiseID      uint32 // ID of the last push promise (even), or 0 if there have been no pushes
 	streams               map[uint32]*stream
 	initialWindowSize     int32
 	maxFrameSize          int32
@@ -477,8 +477,14 @@ func (sc *serverConn) state(streamID uint32) (streamState, *stream) {
 	// a client sends a HEADERS frame on stream 7 without ever sending a
 	// frame on stream 5, then stream 5 transitions to the "closed"
 	// state when the first frame for stream 7 is sent or received."
-	if streamID <= sc.maxStreamID {
-		return stateClosed, nil
+	if streamID%2 == 1 {
+		if streamID <= sc.maxClientStreamID {
+			return stateClosed, nil
+		}
+	} else {
+		if streamID <= sc.maxPushPromiseID {
+			return stateClosed, nil
+		}
 	}
 	return stateIdle, nil
 }
@@ -1011,7 +1017,7 @@ func (sc *serverConn) scheduleFrameWrite() {
 			sc.needToSendGoAway = false
 			sc.startFrameWrite(FrameWriteRequest{
 				write: &writeGoAway{
-					maxStreamID: sc.maxStreamID,
+					maxStreamID: sc.maxClientStreamID,
 					code:        sc.goAwayCode,
 				},
 			})
@@ -1495,10 +1501,10 @@ func (sc *serverConn) processHeaders(f *MetaHeadersFrame) error {
 	// endpoint has opened or reserved. [...]  An endpoint that
 	// receives an unexpected stream identifier MUST respond with
 	// a connection error (Section 5.4.1) of type PROTOCOL_ERROR.
-	if id <= sc.maxStreamID {
+	if id <= sc.maxClientStreamID {
 		return ConnectionError(ErrCodeProtocol)
 	}
-	sc.maxStreamID = id
+	sc.maxClientStreamID = id
 
 	if sc.idleTimer != nil {
 		sc.idleTimer.Stop()

http2/server_push_test.go

@@ -381,3 +381,49 @@ func TestServer_Push_RejectForbiddenHeader(t *testing.T) {
 			return nil
 		})
 }
+
+func TestServer_Push_StateTransitions(t *testing.T) {
+	const body = "foo"
+
+	startedPromise := make(chan bool)
+	finishedPush := make(chan bool)
+	st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+		switch r.URL.RequestURI() {
+		case "/":
+			if err := w.(http.Pusher).Push("/pushed", nil); err != nil {
+				t.Errorf("Push error: %v", err)
+			}
+			close(startedPromise)
+			// Don't finish this request until the push finishes so we don't
+			// nondeterministically interleave output frames with the push.
+			<-finishedPush
+		}
+		w.Header().Set("Content-Type", "text/html")
+		w.Header().Set("Content-Length", strconv.Itoa(len(body)))
+		w.WriteHeader(200)
+		io.WriteString(w, body)
+	})
+	defer st.Close()
+
+	st.greet()
+	if st.stream(2) != nil {
+		t.Fatal("stream 2 should be empty")
+	}
+	if got, want := st.streamState(2), stateIdle; got != want {
+		t.Fatalf("streamState(2)=%v, want %v", got, want)
+	}
+	getSlash(st)
+	<-startedPromise
+	if got, want := st.streamState(2), stateHalfClosedRemote; got != want {
+		t.Fatalf("streamState(2)=%v, want %v", got, want)
+	}
+	st.wantPushPromise()
+	st.wantHeaders()
+	if df := st.wantData(); !df.StreamEnded() {
+		t.Fatal("expected END_STREAM flag on DATA")
+	}
+	if got, want := st.streamState(2), stateClosed; got != want {
+		t.Fatalf("streamState(2)=%v, want %v", got, want)
+	}
+	close(finishedPush)
+}