fix: bound response writes so a stalled client can't wedge a thread forever

caddy/caddy.go

@@ -19,6 +19,11 @@ const (
 	// reads when the directive is omitted; mirrors nginx's client_body_timeout.
 	// Set request_body_timeout to 0 to disable.
 	defaultRequestBodyTimeout = caddy.Duration(60 * time.Second)
+
+	// defaultResponseWriteTimeout is the idle timeout applied to response
+	// writes when the directive is omitted; mirrors nginx's send_timeout.
+	// Set response_write_timeout to 0 to disable.
+	defaultResponseWriteTimeout = caddy.Duration(60 * time.Second)
 )
 
 func init() {

caddy/config_test.go

@@ -129,6 +129,34 @@ func TestModuleRequestBodyTimeoutDisabled(t *testing.T) {
 	require.Equal(t, caddy.Duration(0), *module.RequestBodyTimeout)
 }
 
+func TestModuleResponseWriteTimeout(t *testing.T) {
+	d := caddyfile.NewTestDispenser(`
+	{
+		php {
+			response_write_timeout 5s
+		}
+	}`)
+	module := &FrankenPHPModule{}
+
+	require.NoError(t, module.UnmarshalCaddyfile(d))
+	require.NotNil(t, module.ResponseWriteTimeout)
+	require.Equal(t, caddy.Duration(5*time.Second), *module.ResponseWriteTimeout)
+}
+
+func TestModuleResponseWriteTimeoutDisabled(t *testing.T) {
+	d := caddyfile.NewTestDispenser(`
+	{
+		php {
+			response_write_timeout 0
+		}
+	}`)
+	module := &FrankenPHPModule{}
+
+	require.NoError(t, module.UnmarshalCaddyfile(d))
+	require.NotNil(t, module.ResponseWriteTimeout)
+	require.Equal(t, caddy.Duration(0), *module.ResponseWriteTimeout)
+}
+
 func TestModuleWorkerDuplicateFilenamesFail(t *testing.T) {
 	// Create a test configuration with duplicate worker filenames
 	configWithDuplicateFilenames := `

caddy/module.go

@@ -50,6 +50,8 @@ type FrankenPHPModule struct {
 	RouteGroup string `json:"route_group,omitempty"`
 	// RequestBodyTimeout is an idle timeout on request body reads: a stalled (slow POST) client is cut off while a steady upload of any size succeeds. Defaults to 60s when omitted; set to 0 to disable.
 	RequestBodyTimeout *caddy.Duration `json:"request_body_timeout,omitempty"`
+	// ResponseWriteTimeout is an idle timeout on response writes: a stalled or dead client is cut off instead of blocking the PHP thread forever. Defaults to 60s when omitted; set to 0 to disable.
+	ResponseWriteTimeout *caddy.Duration `json:"response_write_timeout,omitempty"`
 
 	resolvedDocumentRoot        string
 	preparedEnv                 frankenphp.PreparedEnv
@@ -137,6 +139,14 @@ func (f *FrankenPHPModule) Provision(ctx caddy.Context) error {
 		f.requestOptions = append(f.requestOptions, frankenphp.WithRequestBodyTimeout(time.Duration(*f.RequestBodyTimeout)))
 	}
 
+	if f.ResponseWriteTimeout == nil {
+		f.ResponseWriteTimeout = new(defaultResponseWriteTimeout)
+	}
+
+	if *f.ResponseWriteTimeout > 0 {
+		f.requestOptions = append(f.requestOptions, frankenphp.WithResponseWriteTimeout(time.Duration(*f.ResponseWriteTimeout)))
+	}
+
 	if f.ResolveRootSymlink == nil {
 		f.ResolveRootSymlink = new(true)
 	}
@@ -337,8 +347,21 @@ func (f *FrankenPHPModule) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				}
 				f.RequestBodyTimeout = new(caddy.Duration(v))
 
+			case "response_write_timeout":
+				if !d.NextArg() {
+					return d.ArgErr()
+				}
+				v, err := caddy.ParseDuration(d.Val())
+				if err != nil {
+					return err
+				}
+				if d.NextArg() {
+					return d.ArgErr()
+				}
+				f.ResponseWriteTimeout = new(caddy.Duration(v))
+
 			default:
-				return wrongSubDirectiveError("php or php_server", "hot_reload, name, root, split, env, resolve_root_symlink, request_body_timeout, worker", d.Val())
+				return wrongSubDirectiveError("php or php_server", "hot_reload, name, root, split, env, resolve_root_symlink, request_body_timeout, response_write_timeout, worker", d.Val())
 			}
 		}
 	}

context.go

@@ -26,6 +26,8 @@ type frankenPHPContext struct {
 
 	// idle timeout per body read; zero disables it
 	requestBodyTimeout time.Duration
+	// idle timeout per response write; zero disables it
+	responseWriteTimeout time.Duration
 
 	docURI         string
 	pathInfo       string

docs/config.md

@@ -190,6 +190,7 @@ php_server [<matcher>] {
 	env <key> <value> # Sets an extra environment variable to the given value. Can be specified more than once for multiple environment variables.
 	file_server off # Disables the built-in file_server directive.
 	request_body_timeout <duration> # Sets an idle timeout on request body reads: a stalled (slow POST) client is cut off while a steady upload of any size succeeds. Default: 60s. Set to 0 to disable.
+	response_write_timeout <duration> # Sets an idle timeout on response writes: a stalled or dead client is cut off instead of blocking the PHP thread forever. Default: 60s. Set to 0 to disable.
 	worker { # Creates a worker specific to this server. Can be specified more than once for multiple workers.
 		file <path> # Sets the path to the worker script, can be relative to the php_server root
 		num <num> # Sets the number of PHP threads to start, defaults to 2x the number of available

docs/security.md

@@ -52,6 +52,7 @@ These are the surfaces FrankenPHP owns. A vulnerability here is a FrankenPHP vul
 - **Caddy admin API**: the `/frankenphp/workers/restart` and `/frankenphp/threads` endpoints, exposed through Caddy's admin API (which listens on `localhost:2019` by default). Exposing that endpoint beyond localhost is an operator decision.
 - **Trusted proxy handling**: incoming `X-Forwarded-*` headers always reach PHP as tainted `$_SERVER['HTTP_X_FORWARDED_*']` values; they are only trusted to derive the real client IP and scheme when [`trusted_proxies`](production.md#running-behind-a-reverse-proxy) is configured.
 - **Slow request bodies**: a client that announces a body then dribbles or stalls it holds the handling thread for the duration. With a bounded thread pool, enough such connections exhaust it (slow-POST DoS). FrankenPHP applies a 60s idle timeout on body reads by default ([`request_body_timeout`](config.md#caddyfile-config)), resetting the deadline before each read so a steady upload of any size succeeds while a stalled one is cut off and the thread released.
+- **Stalled response writes**: a client that stops reading the response (slow-read DoS) holds the handling thread in a blocking write indefinitely: `frankenphp_force_kill_thread` cannot interrupt it, since the stall isn't a blocking syscall but a write parked in Go's non-blocking netpoller. FrankenPHP applies a 60s idle timeout on response writes by default ([`response_write_timeout`](config.md#caddyfile-config)), resetting the deadline before each write so the thread is released once a peer goes unresponsive.
 
 ## Out of scope
 

frankenphp.go

@@ -437,23 +437,46 @@ func go_ub_write(threadIndex C.uintptr_t, cBuf *C.char, length C.size_t) (C.size
 	}
 
 	var writer io.Writer
+	var rc *http.ResponseController
 	if fc.responseWriter == nil {
 		var b bytes.Buffer
 		// log the output of the worker
 		writer = &b
 	} else {
 		writer = fc.responseWriter
+
+		if fc.responseWriteTimeout > 0 {
+			if fc.responseController == nil {
+				fc.responseController = http.NewResponseController(fc.responseWriter)
+			}
+			rc = fc.responseController
+			_ = rc.SetWriteDeadline(time.Now().Add(fc.responseWriteTimeout))
+		}
 	}
 
 	var ctx context.Context
 
 	i, e := writer.Write(unsafe.Slice((*byte)(unsafe.Pointer(cBuf)), length))
+
+	if rc != nil {
+		_ = rc.SetWriteDeadline(time.Time{})
+	}
+
 	if e != nil {
 		ctx = thread.context()
 
 		if fc.logger.Enabled(ctx, slog.LevelWarn) {
 			fc.logger.LogAttrs(ctx, slog.LevelWarn, "write error", slog.Any("error", e))
 		}
+
+		if errors.Is(e, os.ErrDeadlineExceeded) {
+			// The peer stopped reading and the write timeout fired: treat this
+			// exactly like a genuine disconnect (rather than fc.clientHasClosed(),
+			// which only reflects the request context and won't have fired yet) so
+			// php_handle_aborted_connection() runs instead of the script quietly
+			// continuing to write into a stalled connection on every future write.
+			return C.size_t(i), C.bool(true)
+		}
 	}
 
 	if fc.responseWriter == nil {

requestoptions.go

@@ -192,6 +192,20 @@ func WithRequestBodyTimeout(timeout time.Duration) RequestOption {
 	}
 }
 
+// WithResponseWriteTimeout sets an idle timeout on response writes: a client
+// that stops reading (a stalled or dead peer) is cut off instead of blocking
+// the PHP thread forever, since frankenphp_force_kill_thread cannot interrupt
+// a write parked in Go's non-blocking netpoller. Zero (the default) disables
+// it. Requires a ResponseWriter that exposes a write deadline (net/http and
+// Caddy do); otherwise the write has no timeout.
+func WithResponseWriteTimeout(timeout time.Duration) RequestOption {
+	return func(o *frankenPHPContext) error {
+		o.responseWriteTimeout = timeout
+
+		return nil
+	}
+}
+
 // WithWorkerName sets the worker that should handle the request
 func WithWorkerName(name string) RequestOption {
 	return func(o *frankenPHPContext) error {

responsewritetimeout_test.go

@@ -0,0 +1,58 @@
+package frankenphp_test
+
+import (
+	"fmt"
+	"net"
+	"net/http"
+	"os"
+	"testing"
+	"time"
+
+	"github.com/dunglas/frankenphp"
+	"github.com/stretchr/testify/require"
+)
+
+// TestResponseWriteTimeout proves that WithResponseWriteTimeout bounds a
+// stalled reader: the client never reads the response, so the server's socket
+// send buffer fills up and the underlying Write() blocks. Without the option
+// the PHP thread would block in ub_write forever, since
+// frankenphp_force_kill_thread cannot interrupt a write parked in Go's
+// non-blocking netpoller (see php/frankenphp#2553, php/frankenphp#2573); with
+// it, the write is cut off and the handler returns.
+func TestResponseWriteTimeout(t *testing.T) {
+	require.NoError(t, frankenphp.Init())
+	defer frankenphp.Shutdown()
+
+	cwd, _ := os.Getwd()
+	done := make(chan struct{})
+	handler := func(w http.ResponseWriter, r *http.Request) {
+		defer close(done)
+		req, err := frankenphp.NewRequestWithContext(r,
+			frankenphp.WithRequestDocumentRoot(cwd+"/testdata/", false),
+			frankenphp.WithResponseWriteTimeout(300*time.Millisecond),
+		)
+		require.NoError(t, err)
+		require.NoError(t, frankenphp.ServeHTTP(w, req))
+	}
+
+	ts := newRawServer(t, handler)
+	defer ts.Close()
+
+	conn, err := net.Dial("tcp", ts.Addr())
+	require.NoError(t, err)
+	defer func() { _ = conn.Close() }()
+
+	// Shrink the client's receive window so the server's send buffer fills
+	// after only a handful of writes instead of relying on OS defaults.
+	require.NoError(t, conn.(*net.TCPConn).SetReadBuffer(1024))
+
+	_, err = fmt.Fprintf(conn, "GET /slow-write.php HTTP/1.1\r\nHost: %s\r\nConnection: close\r\n\r\n", ts.Addr())
+	require.NoError(t, err)
+
+	// Never read the response: that's the stalled reader.
+	select {
+	case <-done:
+	case <-time.After(5 * time.Second):
+		t.Fatal("the write timeout did not release the PHP thread")
+	}
+}

testdata/slow-write.php

@@ -0,0 +1,11 @@
+<?php
+
+// Write continuously so a stalled reader eventually blocks the underlying
+// socket write. Capped at 10s so an unpatched build fails the test instead of
+// hanging it forever.
+ob_implicit_flush(true);
+$start = microtime(true);
+while (microtime(true) - $start < 10) {
+    echo str_repeat('x', 65536);
+}
+echo 'did not time out';