Skip to content

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

Open
lazerg wants to merge 2 commits into
php:PHP-8.4from
lazerg:fix/issue-23425-cli-server-send-headers
Open

sapi/cli: check php_cli_server_client_send_through() return value#23428
lazerg wants to merge 2 commits into
php:PHP-8.4from
lazerg:fix/issue-23425-cli-server-send-headers

Conversation

@lazerg

@lazerg lazerg commented Aug 23, 2026

Copy link
Copy Markdown
Contributor

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

@lazerg
lazerg force-pushed the fix/issue-23425-cli-server-send-headers branch from 16edc5e to fbf3fbe Compare August 23, 2026 21:01
Comment thread sapi/cli/php_cli_server.c

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;

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants