diff --git a/frankenphp.go b/frankenphp.go index f477aceb1b..ad2dedc42a 100644 --- a/frankenphp.go +++ b/frankenphp.go @@ -643,8 +643,13 @@ func go_read_post(threadIndex C.uintptr_t, cBuf *C.char, countBytes C.size_t) (r return 0 } + // The read deadline is set on the responseWriter, which is only valid until + // the response is finished. A script that finishes the request (e.g. via + // frankenphp_finish_request()) and then reads the body would otherwise set a + // deadline on a finalized HTTP/2 stream, dereferencing a nil pointer and + // crashing the process. See https://github.com/php/frankenphp/issues/2535. var rc *http.ResponseController - if fc.requestBodyTimeout > 0 { + if fc.requestBodyTimeout > 0 && !fc.isDone { if fc.responseController == nil { fc.responseController = http.NewResponseController(fc.responseWriter) } diff --git a/requestbodytimeout_test.go b/requestbodytimeout_test.go index 4b8c140759..14912f8a23 100644 --- a/requestbodytimeout_test.go +++ b/requestbodytimeout_test.go @@ -1,18 +1,51 @@ package frankenphp_test import ( + "context" + "crypto/tls" "fmt" "io" "net" "net/http" "os" + "strings" "testing" "time" "github.com/dunglas/frankenphp" "github.com/stretchr/testify/require" + "golang.org/x/net/http2" ) +// newH2CServer starts a cleartext HTTP/2 (h2c) server for handler and returns +// its address and a matching client. h2c drives the SetReadDeadline path that +// only HTTP/2 exercises. The listener and server are closed via t.Cleanup. +func newH2CServer(t *testing.T, handler http.HandlerFunc) (addr string, client *http.Client) { + t.Helper() + + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + protocols := new(http.Protocols) + protocols.SetUnencryptedHTTP2(true) + srv := &http.Server{Handler: handler, Protocols: protocols} + + go func() { _ = srv.Serve(ln) }() + t.Cleanup(func() { + _ = srv.Close() + _ = ln.Close() + }) + + client = &http.Client{Transport: &http2.Transport{ + AllowHTTP: true, + DialTLSContext: func(_ context.Context, network, addr string, _ *tls.Config) (net.Conn, error) { + return net.Dial(network, addr) + }, + }} + + return ln.Addr().String(), client +} + // TestRequestBodyTimeout proves that WithRequestBodyTimeout bounds a slow-POST // client: it announces a large Content-Length, then stalls without sending the // body. Without the option the PHP thread would block in Body.Read until the @@ -59,6 +92,90 @@ func TestRequestBodyTimeout(t *testing.T) { require.Contains(t, string(resp), "read=0") } +// TestRequestBodyTimeoutHTTP2 is the HTTP/2 counterpart of the test above: over +// HTTP/2 the deadline lands on the stream (via x/net) rather than the net.Conn, +// so this exercises the SetReadDeadline path that HTTP/1 never touches. A slow +// POST that trips the idle timeout must bound the read and return cleanly. +// The nil-dereference crash of php/frankenphp#2535 needs the writer to be used +// after its stream is finalized; see TestSetReadDeadlineRecoversFromPanic. +func TestRequestBodyTimeoutHTTP2(t *testing.T) { + require.NoError(t, frankenphp.Init()) + defer frankenphp.Shutdown() + + cwd, _ := os.Getwd() + handler := func(w http.ResponseWriter, r *http.Request) { + req, err := frankenphp.NewRequestWithContext(r, + frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false), + frankenphp.WithRequestBodyTimeout(300*time.Millisecond), + ) + require.NoError(t, err) + require.NoError(t, frankenphp.ServeHTTP(w, req)) + } + + addr, client := newH2CServer(t, handler) + + // A body that never sends data: the server blocks in Body.Read until the + // idle timeout fires. Close the writer once the request returns. + pr, pw := io.Pipe() + defer func() { _ = pw.Close() }() + + req, err := http.NewRequest(http.MethodPost, "http://"+addr+"/read-input.php", pr) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/octet-stream") + + start := time.Now() + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + elapsed := time.Since(start) + + require.Less(t, elapsed, 4*time.Second, "slow body must be bounded by the timeout") + require.Equal(t, http.StatusOK, resp.StatusCode) + require.Contains(t, string(body), "read=0") +} + +// TestFinishRequestThenReadBodyHTTP2 reproduces php/frankenphp#2535: a script +// that calls frankenphp_finish_request() and then reads php://input triggers +// go_read_post after the HTTP/2 responseWriter has been finalized. Setting a +// read deadline on that dead writer would dereference a nil pointer and crash +// the whole process. enable_post_data_reading=Off defers the body read until +// the explicit php://input access, i.e. after the request is finished. +func TestFinishRequestThenReadBodyHTTP2(t *testing.T) { + iniDir := t.TempDir() + require.NoError(t, os.WriteFile(iniDir+"/php.ini", []byte("enable_post_data_reading=Off\n"), 0o600)) + t.Setenv("PHPRC", iniDir+"/php.ini") + + require.NoError(t, frankenphp.Init()) + defer frankenphp.Shutdown() + + cwd, _ := os.Getwd() + handler := func(w http.ResponseWriter, r *http.Request) { + req, err := frankenphp.NewRequestWithContext(r, + frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false), + frankenphp.WithRequestBodyTimeout(300*time.Millisecond), + ) + require.NoError(t, err) + require.NoError(t, frankenphp.ServeHTTP(w, req)) + } + + addr, client := newH2CServer(t, handler) + + req, err := http.NewRequest(http.MethodPost, "http://"+addr+"/finish-then-read-input.php", strings.NewReader("hello world")) + require.NoError(t, err) + req.Header.Set("Content-Type", "application/octet-stream") + + resp, err := client.Do(req) + require.NoError(t, err) + defer func() { _ = resp.Body.Close() }() + + _, err = io.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, http.StatusOK, resp.StatusCode) +} + // rawServer is a minimal HTTP server exposing its listener address so a test // can drive it with a raw TCP connection (needed to simulate a stalled body). type rawServer struct { diff --git a/testdata/finish-then-read-input.php b/testdata/finish-then-read-input.php new file mode 100644 index 0000000000..9d26990a93 --- /dev/null +++ b/testdata/finish-then-read-input.php @@ -0,0 +1,12 @@ +