From db92ab64c3d64a2eed1a18dca998b02dc41cc9d5 Mon Sep 17 00:00:00 2001 From: Kevin Dunglas Date: Mon, 20 Jul 2026 13:30:39 +0200 Subject: [PATCH 1/3] fix: don't set request body read deadline after the request is finished frankenphp_finish_request() (or any early context close) finalizes the HTTP/2 responseWriter while the PHP script keeps running. A later php://input read then set a read deadline on the dead stream, which dereferences a nil pointer in golang.org/x/net and crashes the whole process. Skip the deadline once the context is done. Closes #2535 --- frankenphp.go | 7 +- requestbodytimeout_test.go | 115 ++++++++++++++++++++++++++++ testdata/finish-then-read-input.php | 12 +++ 3 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 testdata/finish-then-read-input.php 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..346f7539f4 100644 --- a/requestbodytimeout_test.go +++ b/requestbodytimeout_test.go @@ -1,16 +1,21 @@ 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" + "golang.org/x/net/http2/h2c" ) // TestRequestBodyTimeout proves that WithRequestBodyTimeout bounds a slow-POST @@ -59,6 +64,116 @@ 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)) + } + + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + h2s := &http2.Server{} + srv := &http.Server{Handler: h2c.NewHandler(http.HandlerFunc(handler), h2s)} + go func() { _ = srv.Serve(ln) }() + defer func() { _ = srv.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) + }, + }} + + // 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://"+ln.Addr().String()+"/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)) + } + + ln, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + h2s := &http2.Server{} + srv := &http.Server{Handler: h2c.NewHandler(http.HandlerFunc(handler), h2s)} + go func() { _ = srv.Serve(ln) }() + defer func() { _ = srv.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) + }, + }} + + req, err := http.NewRequest(http.MethodPost, "http://"+ln.Addr().String()+"/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 @@ + Date: Mon, 20 Jul 2026 15:13:19 +0200 Subject: [PATCH 2/3] test: replace deprecated h2c with stdlib Protocols for HTTP/2 --- requestbodytimeout_test.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/requestbodytimeout_test.go b/requestbodytimeout_test.go index 346f7539f4..de4f97f930 100644 --- a/requestbodytimeout_test.go +++ b/requestbodytimeout_test.go @@ -15,9 +15,18 @@ import ( "github.com/dunglas/frankenphp" "github.com/stretchr/testify/require" "golang.org/x/net/http2" - "golang.org/x/net/http2/h2c" ) +// h2cServer builds an HTTP server that speaks cleartext HTTP/2 (h2c) using the +// stdlib Protocols field, so a slow-body test drives the SetReadDeadline path +// that only HTTP/2 exercises. +func h2cServer(handler http.HandlerFunc) *http.Server { + protocols := new(http.Protocols) + protocols.SetUnencryptedHTTP2(true) + + return &http.Server{Handler: handler, Protocols: protocols} +} + // 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 @@ -87,8 +96,7 @@ func TestRequestBodyTimeoutHTTP2(t *testing.T) { ln, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - h2s := &http2.Server{} - srv := &http.Server{Handler: h2c.NewHandler(http.HandlerFunc(handler), h2s)} + srv := h2cServer(handler) go func() { _ = srv.Serve(ln) }() defer func() { _ = srv.Close() }() @@ -149,8 +157,7 @@ func TestFinishRequestThenReadBodyHTTP2(t *testing.T) { ln, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - h2s := &http2.Server{} - srv := &http.Server{Handler: h2c.NewHandler(http.HandlerFunc(handler), h2s)} + srv := h2cServer(handler) go func() { _ = srv.Serve(ln) }() defer func() { _ = srv.Close() }() From 8f0daeadde255874f125008ee9bca88357fd09b6 Mon Sep 17 00:00:00 2001 From: Kevin Dunglas Date: Mon, 20 Jul 2026 15:19:39 +0200 Subject: [PATCH 3/3] test: fold h2c server setup into a t.Cleanup helper --- requestbodytimeout_test.go | 61 +++++++++++++++++--------------------- 1 file changed, 28 insertions(+), 33 deletions(-) diff --git a/requestbodytimeout_test.go b/requestbodytimeout_test.go index de4f97f930..14912f8a23 100644 --- a/requestbodytimeout_test.go +++ b/requestbodytimeout_test.go @@ -17,14 +17,33 @@ import ( "golang.org/x/net/http2" ) -// h2cServer builds an HTTP server that speaks cleartext HTTP/2 (h2c) using the -// stdlib Protocols field, so a slow-body test drives the SetReadDeadline path -// that only HTTP/2 exercises. -func h2cServer(handler http.HandlerFunc) *http.Server { +// 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} - return &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 @@ -93,26 +112,14 @@ func TestRequestBodyTimeoutHTTP2(t *testing.T) { require.NoError(t, frankenphp.ServeHTTP(w, req)) } - ln, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) - - srv := h2cServer(handler) - go func() { _ = srv.Serve(ln) }() - defer func() { _ = srv.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) - }, - }} + 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://"+ln.Addr().String()+"/read-input.php", pr) + req, err := http.NewRequest(http.MethodPost, "http://"+addr+"/read-input.php", pr) require.NoError(t, err) req.Header.Set("Content-Type", "application/octet-stream") @@ -154,21 +161,9 @@ func TestFinishRequestThenReadBodyHTTP2(t *testing.T) { require.NoError(t, frankenphp.ServeHTTP(w, req)) } - ln, err := net.Listen("tcp", "127.0.0.1:0") - require.NoError(t, err) - - srv := h2cServer(handler) - go func() { _ = srv.Serve(ln) }() - defer func() { _ = srv.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) - }, - }} + addr, client := newH2CServer(t, handler) - req, err := http.NewRequest(http.MethodPost, "http://"+ln.Addr().String()+"/finish-then-read-input.php", strings.NewReader("hello world")) + 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")