http2 server crashes with an unhandled panic when SetReadDeadline is called for a request with no body

[tkashem]

What version of Go are you using (go version)?

$ go version
go version go1.21.1 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

go env Output

$ go env
GOARCH='amd64'
GOOS='linux'

What did you do?

setting up per handler read timeout:

   ctrl := http.NewResponseController(w)
   ctrl.SetReadDeadline(t)

What did you expect to see?

No panic

What did you see instead?

For a request with no body, setting up per handler read timeout results in a panic that crashes the server process. I am seeing the following panic:

=== RUN   TestPerHandlerReadTimeoutWithHTTP2RequestWithNoBody
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x97c781]

goroutine 14 [running]:
net/http.(*http2pipe).CloseWithError(...)
	/usr/lib/go/src/net/http/h2_bundle.go:3791
net/http.(*http2stream).onReadTimeout(0xc0003de000)
	/usr/lib/go/src/net/http/h2_bundle.go:5722 +0xe1
created by time.goFunc
	/usr/lib/go/src/time/sleep.go:176 +0x45
FAIL	k8s.io/apiserver/pkg/endpoints/filters	0.196s
FAIL

Here is the test I am using:

func TestPerHandlerReadTimeoutWithHTTP2RequestWithNoBody(t *testing.T) {
	blockedCh, doneCh := make(chan struct{}), make(chan struct{})
	server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
		defer close(doneCh)

		ctrl := http.NewResponseController(w)
		if err := ctrl.SetReadDeadline(time.Now().Add(100 * time.Millisecond)); err != nil {
			t.Errorf("expected no error from SetReadDeadline, but got: %v", err)
			return
		}

		<-blockedCh
	}))
	defer server.Close()
	server.EnableHTTP2 = true
	server.StartTLS()

	client := server.Client()
	_, err := client.Get(server.URL + "/foo")
	if err == nil {
		t.Errorf("expected an error")
	}

	close(blockedCh)
	<-doneCh
}

the body associated with an http2 stream may be nil here:

go/src/net/http/h2_bundle.go

Lines 5821 to 5822 in 7117819

	st.body = req.Body.(*http2requestBody).pipe // may be nil
	st.declBodyBytes = req.ContentLength

The implementation of http2responseWriter.SetReadDeadline does not check whether the body field of the associated stream is nil:

go/src/net/http/h2_bundle.go

Lines 6547 to 6548 in 7117819

	} else if st.readDeadline == nil {
	st.readDeadline = time.AfterFunc(deadline.Sub(time.Now()), st.onReadTimeout)

In other places, i see a nil check for the body of the stream:

go/src/net/http/h2_bundle.go

Lines 5841 to 5843 in 7117819

	if st.body != nil {
	st.readDeadline = time.AfterFunc(sc.hs.ReadTimeout, st.onReadTimeout)
	}

we can avoid the panic by adding the following check for content length:

		if req.ContentLength != 0 {
			if err := ctrl.SetReadDeadline(time.Now().Add(100 * time.Millisecond)); err != nil {
				t.Errorf("expected no error from SetReadDeadline, but got: %v", err)
				return
			}
		}

is checking the ContentLength of the request before calling SetReadDeadline the expected behavior? I did not come across any doc for this behavior

I think it may be more appropriate to add a nil check inside SetReadDeadline:

if st.body == nil {
   return nil
}

I would appreciate your thoughts on this, thanks!

[tkashem]

#58237 - looks like it has been reported and the fix is in place, thanks!

[bcmills]

Duplicate of #58237