sapi/cli: check php_cli_server_client_send_through() return value

[lazerg]

sapi_cli_server_send_headers() sends the header buffer through php_cli_server_client_send_through() but drops the return value, so it always reports SAPI_HEADER_SENT_SUCCESSFULLY, even when the send fails and php_handle_aborted_connection() doesn't bail out (ignore_user_abort=1). This compares the send result against the buffer length and returns SAPI_HEADER_SEND_FAILED when it comes up short.

Fixes #23425

[devnexen]

note that php_cli_server_client_send_through returns the number of bytes left on failure so it s wrong here if nothing was consumed.

[lazerg]

Good catch. On failure the callee returned nbytes_left, which equals str_len when nothing was sent, same value as success. Fixed in e2f8bcd: it now returns bytes actually sent (str_len - nbytes_left) on both paths, so this comparison is unambiguous.

[devnexen]

I suggest the following changes

@@ typedef struct php_cli_server_client {
      bool request_read;
+     bool headers_written;
      zend_string *current_header_name;

@@ static void php_cli_server_client_ctor(
      client->request_read = false;
+     client->headers_written = false;

@@ static int sapi_cli_server_send_headers(sapi_headers_struct *sapi_headers)
-     if (client == NULL || SG(request_info).no_headers) {
+     if (client == NULL || SG(request_info).no_headers || client->headers_written) {
              return SAPI_HEADER_SENT_SUCCESSFULLY;
      }

      bool sent = php_cli_server_client_send_through(client, ZSTR_VAL(buffer.s), buffer_len) == buffer_len;

+     client->headers_written = true;
      smart_str_free(&buffer);
      return sent ? SAPI_HEADER_SENT_SUCCESSFULLY : SAPI_HEADER_SEND_FAILED;

[lazerg]

Applied as-is in f502d8a. This closes a gap my own fix opened: on SAPI_HEADER_SEND_FAILED, sapi_send_headers() in main/SAPI.c resets headers_sent back to false, so every later output write re-enters php_header() and would call sapi_cli_server_send_headers() again on the same dead socket, rebuilding and resending the whole header block each time. Before this PR, that path never ran because the return value was always success, so headers_sent never got reset. The headers_written guard makes the send attempt at most once per request. gh23425.phpt still passes (single write, so it only hits the guard's write path once) and the full sapi/cli/tests/ suite passes, 95/95.

[Sjord]

Looks good to me.

I forgot to mention in the issue that the return value of php_cli_server_client_send_through was incorrect, sorry about that.

[devnexen]

I would like to see a test, I doubt returning SAPI_HEADER_SEND_FAILED like this is the way to go.

Edit: need to make sure headers are effectively written too.

[lazerg]

Added a test in 683e87b: it opens a real connection, sends a full request, then hard-resets it (SO_LINGER=0) while the script sleeps with ignore_user_abort(true), so the header write fails on a live socket, not a mock. Checked both directions locally: it fails against the pre-fix code and passes against the fix, 8/8 runs.

On the second point: the failure does change behavior downstream, not just bookkeeping. sapi_send_headers() resets headers_sent to false on SAPI_HEADER_SEND_FAILED, so php_header() returns false and main/output.c sets PHP_OUTPUT_DISABLED, which makes the next output write skip the dead socket instead of retrying it. The test checks headers_sent() after the failed write for exactly that reason.

[Sjord]

Nice.

It is a bit unfortunate that headers_sent pretends that the only reason it could return false is that headers have not been sent yet, instead of trying to send them and failing.

headers_sent() will return false if no HTTP headers have already been sent or true otherwise.

[Sjord]

@devnexen also had this in his test:

if (PHP_OS_FAMILY === "Windows") die("skip SO_LINGER reset behaviour differs on Windows");

It looks like that may be needed here, but I am not sure about the SO_LINGER behavior on Windows.

[devnexen]

if CI is green

[devnexen]

Thanks !