http2: prevent Server from sending status 100 header after anything else

Since Go's HTTP/2 Handlers can read & write concurrently, it was
previously possible for a Handler to get an HTTP Request with "Expect:
100-continue" and to first Write some data, then read some data, and
the read of the data was triggering the server's automatic
"100-continue" response path.  But if the server had already replied
with a non-100 HEADERS frame, sending a HEADERS frame with :status 100
later is in violation of the spec.

Fix it by tracking whether we've already replied with something, and
ignoring the 100-continue when it's inappropriate.

Updates golang/go#14030

Change-Id: I6745ba0f257a31eaf63816e4263cadbef5a896a2
Reviewed-on: https://go-review.googlesource.com/23311
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>

http2/server.go

@@ -461,6 +461,7 @@ type stream struct {
 	sentReset        bool // only true once detached from streams map
 	gotReset         bool // only true once detacted from streams map
 	gotTrailerHeader bool // HEADER frame for trailers was seen
+	wroteHeaders     bool // whether we wrote headers (not status 100)
 	reqBuf           []byte
 
 	trailer    http.Header // accumulated trailers
@@ -848,7 +849,23 @@ func (sc *serverConn) writeFrameFromHandler(wm frameWriteMsg) error {
 // If you're not on the serve goroutine, use writeFrameFromHandler instead.
 func (sc *serverConn) writeFrame(wm frameWriteMsg) {
 	sc.serveG.check()
-	sc.writeSched.add(wm)
+
+	var ignoreWrite bool
+
+	// Don't send a 100-continue response if we've already sent headers.
+	// See golang.org/issue/14030.
+	switch wm.write.(type) {
+	case *writeResHeaders:
+		wm.stream.wroteHeaders = true
+	case write100ContinueHeadersFrame:
+		if wm.stream.wroteHeaders {
+			ignoreWrite = true
+		}
+	}
+
+	if !ignoreWrite {
+		sc.writeSched.add(wm)
+	}
 	sc.scheduleFrameWrite()
 }
 

http2/server_test.go

@@ -3244,3 +3244,54 @@ func TestCheckValidHTTP2Request(t *testing.T) {
 		}
 	}
 }
+
+// golang.org/issue/14030
+func TestExpect100ContinueAfterHandlerWrites(t *testing.T) {
+	const msg = "Hello"
+	const msg2 = "World"
+
+	doRead := make(chan bool, 1)
+	defer close(doRead) // fallback cleanup
+
+	st := newServerTester(t, func(w http.ResponseWriter, r *http.Request) {
+		io.WriteString(w, msg)
+		w.(http.Flusher).Flush()
+
+		// Do a read, which might force a 100-continue status to be sent.
+		<-doRead
+		r.Body.Read(make([]byte, 10))
+
+		io.WriteString(w, msg2)
+
+	}, optOnlyServer)
+	defer st.Close()
+
+	tr := &Transport{TLSClientConfig: tlsConfigInsecure}
+	defer tr.CloseIdleConnections()
+
+	req, _ := http.NewRequest("POST", st.ts.URL, io.LimitReader(neverEnding('A'), 2<<20))
+	req.Header.Set("Expect", "100-continue")
+
+	res, err := tr.RoundTrip(req)
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer res.Body.Close()
+
+	buf := make([]byte, len(msg))
+	if _, err := io.ReadFull(res.Body, buf); err != nil {
+		t.Fatal(err)
+	}
+	if string(buf) != msg {
+		t.Fatalf("msg = %q; want %q", buf, msg)
+	}
+
+	doRead <- true
+
+	if _, err := io.ReadFull(res.Body, buf); err != nil {
+		t.Fatal(err)
+	}
+	if string(buf) != msg2 {
+		t.Fatalf("second msg = %q; want %q", buf, msg2)
+	}
+}