http2: avoid panic on h2c upgrade failure

When performing an h2c upgrade, we would panic if an error occurred reading the client preface. *serverConn.closeStream assumes that a conn with an IdleTimeout > 0 will have an idleTimer, but in the h2c upgrade case the stream may have been created before the timer. Fixes golang/go#67168 Change-Id: I30b5a701c10753ddc344079b9498285f099155cf Reviewed-on: https://go-review.googlesource.com/c/net/+/584255 Reviewed-by: Jonathan Amsterdam <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
golang · May 8, 2024 · 7fa635b · 7fa635b
1 parent d27919b
commit 7fa635b
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 1 deletion.
diff --git a/http2/server.go b/http2/server.go
@@ -1639,7 +1639,7 @@ func (sc *serverConn) closeStream(st *stream, err error) {
 	delete(sc.streams, st.id)
 	if len(sc.streams) == 0 {
 		sc.setConnState(http.StateIdle)
-		if sc.srv.IdleTimeout > 0 {
+		if sc.srv.IdleTimeout > 0 && sc.idleTimer != nil {
 			sc.idleTimer.Reset(sc.srv.IdleTimeout)
 		}
 		if h1ServerKeepAlivesDisabled(sc.hs) {

diff --git a/http2/server_test.go b/http2/server_test.go
@@ -4880,3 +4880,23 @@ func TestServerContinuationAfterInvalidHeader(t *testing.T) {
 		t.Errorf("connection closed with no GOAWAY frame; want one")
 	}
 }
+
+func TestServerUpgradeRequestPrefaceFailure(t *testing.T) {
+	// An h2c upgrade request fails when the client preface is not as expected.
+	s2 := &Server{
+		// Setting IdleTimeout triggers #67168.
+		IdleTimeout: 60 * time.Minute,
+	}
+	c1, c2 := net.Pipe()
+	donec := make(chan struct{})
+	go func() {
+		defer close(donec)
+		s2.ServeConn(c1, &ServeConnOpts{
+			UpgradeRequest: httptest.NewRequest("GET", "/", nil),
+		})
+	}()
+	// The server expects to see the HTTP/2 preface,
+	// but we close the connection instead.
+	c2.Close()
+	<-donec
+}