Skip to content

sapi/cli: check php_cli_server_client_send_through() return value #23428

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lazerg wants to merge 4 commits into from
+52 −5
Closed

sapi/cli: check php_cli_server_client_send_through() return value #23428

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fbf3fbe
sapi/cli: check php_cli_server_client_send_through() return value
lazerg Aug 23, 2026
e2f8bcd
sapi/cli: report bytes actually sent on a failed send_through()
lazerg Aug 23, 2026
683e87b
sapi/cli: add regression test for GH-23425
lazerg Aug 24, 2026
f502d8a
sapi/cli: guard against sending response headers twice
lazerg Aug 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ PHP NEWS
. Fixed bug GH-23301 (Nested "yield from" yields a value twice when the
middle generator delegates again). (Lazizbek Ergashev)

- CLI:
. Fixed bug GH-23425 (sapi_cli_server_send_headers() does not check the
return value of php_cli_server_client_send_through()). (Lazizbek Ergashev)

- DOM:
. Fixed a use-after-free when cloning a DOMNameSpaceNode after
DOMDocument::xinclude(). (iliaal)
Expand Down
14 changes: 9 additions & 5 deletions sapi/cli/php_cli_server.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ typedef struct php_cli_server_client {
zend_string *addr_str;
php_http_parser parser;
bool request_read;
bool headers_written;
zend_string *current_header_name;
zend_string *current_header_value;
enum { HEADER_NONE=0, HEADER_FIELD, HEADER_VALUE } last_header_element;
Expand Down Expand Up @@ -555,7 +556,7 @@ static int sapi_cli_server_send_headers(sapi_headers_struct *sapi_headers) /* {{
sapi_header_struct *h;
zend_llist_position pos;

if (client == NULL || SG(request_info).no_headers) {
if (client == NULL || SG(request_info).no_headers || client->headers_written) {
return SAPI_HEADER_SENT_SUCCESSFULLY;
}

Expand All @@ -578,10 +579,12 @@ static int sapi_cli_server_send_headers(sapi_headers_struct *sapi_headers) /* {{
}
smart_str_appendl(&buffer, "\r\n", 2);

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

@devnexen devnexen Aug 23, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lazerg Aug 23, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 devnexen Aug 24, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lazerg Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


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

@devnexen devnexen Aug 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 lazerg Aug 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
/* }}} */

Expand Down Expand Up @@ -1920,11 +1923,11 @@ static size_t php_cli_server_client_send_through(php_cli_server_client *client,
} else {
/* error or timeout */
php_handle_aborted_connection();
return nbytes_left;
return str_len - nbytes_left;
}
} else {
php_handle_aborted_connection();
return nbytes_left;
return str_len - nbytes_left;
}
}
nbytes_left -= nbytes_sent;
Expand Down Expand Up @@ -1973,6 +1976,7 @@ static void php_cli_server_client_ctor(php_cli_server_client *client, php_cli_se

php_http_parser_init(&client->parser, PHP_HTTP_REQUEST);
client->request_read = false;
client->headers_written = false;

client->last_header_element = HEADER_NONE;
client->current_header_name = NULL;
Expand Down
39 changes: 39 additions & 0 deletions sapi/cli/tests/gh23425.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
--TEST--
GH-23425 (sapi_cli_server_send_headers() does not check the return value of php_cli_server_client_send_through())
--EXTENSIONS--
sockets
--SKIPIF--
<?php
include "skipif.inc";

@Sjord Sjord Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

?>
--FILE--
<?php
include "php_cli_server.inc";

$info = php_cli_server_start(<<<'PHP'
ignore_user_abort(true);
usleep(300000);
header('X-Test: 1');
echo 'x';
file_put_contents(__DIR__ . '/result.txt', headers_sent() ? 'sent' : 'not-sent');
PHP);

// Connect the usual way, then drop to the socket extension only to force a
// hard reset (SO_LINGER=0) instead of a graceful close, so the server's
// header write fails deterministically while the script is still running
// (ignore_user_abort(true)).
$stream = stream_socket_client("tcp://" . PHP_CLI_SERVER_ADDRESS);
$sock = socket_import_stream($stream);
socket_write($sock, "GET /index.php HTTP/1.1\r\nHost: " . PHP_CLI_SERVER_HOSTNAME . "\r\nConnection: close\r\n\r\n");
socket_set_option($sock, SOL_SOCKET, SO_LINGER, ['l_onoff' => 1, 'l_linger' => 0]);
socket_close($sock);

$result_file = $info->docRoot . '/result.txt';
for ($i = 0; $i < 40 && !file_exists($result_file); $i++) {
usleep(50000);
}

echo file_get_contents($result_file), "\n";
?>
--EXPECT--
not-sent
Loading