-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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.
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
Changes from all commits
fbf3fbe
e2f8bcd
683e87b
f502d8a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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; | ||
| } | ||
|
|
||
|
|
@@ -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; | ||
|
|
||
| client->headers_written = true; | ||
| smart_str_free(&buffer); | ||
| return SAPI_HEADER_SENT_SUCCESSFULLY; | ||
| return sent ? SAPI_HEADER_SENT_SUCCESSFULLY : SAPI_HEADER_SEND_FAILED; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
| } | ||
| /* }}} */ | ||
|
|
||
|
|
@@ -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; | ||
|
|
@@ -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; | ||
|
|
||
| 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"; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| ?> | ||
| --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 | ||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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;There was a problem hiding this comment.
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.