From f855b9b66853f2eb69c41323eaaf27614f15c357 Mon Sep 17 00:00:00 2001 From: Sjoerd Langkemper Date: Sat, 17 Oct 2026 11:26:47 +0000 Subject: [PATCH] GH-23423: Return correct return value when write fails php_handle_aborted_connection did not always abort, i.e. if ignore_user_abort=1. So if sending fails and ret is 0, then remaining is never decremented and this loops forever. Fix https://github.com/php/php-src/issues/23423 --- .github/workflows/unit-tests.yml | 2 + NEWS | 4 + sapi/embed/php_embed.c | 1 + tests/unit/Makefile | 4 +- tests/unit/sapi/embed/test_embed_ub_write.c | 100 ++++++++++++++++++++ 5 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 tests/unit/sapi/embed/test_embed_ub_write.c diff --git a/.github/workflows/unit-tests.yml b/.github/workflows/unit-tests.yml index fa79580f42ec..3f974ccd16f3 100644 --- a/.github/workflows/unit-tests.yml +++ b/.github/workflows/unit-tests.yml @@ -5,6 +5,7 @@ on: - 'main/network.c' - 'main/io/**' - 'main/php_io.h' + - 'sapi/embed/**' - 'tests/unit/**' - '.github/workflows/unit-tests.yml' branches: @@ -14,6 +15,7 @@ on: - 'main/network.c' - 'main/io/**' - 'main/php_io.h' + - 'sapi/embed/**' - 'tests/unit/**' - '.github/workflows/unit-tests.yml' branches: diff --git a/NEWS b/NEWS index 4a9a2135ee29..8874a3f1ae69 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,10 @@ PHP NEWS passing an array of CURLFile or CURLStringFile objects to CURLOPT_POSTFIELDS. (Sjoerd Langkemper) +- Embed: + . Fixed infinite loop when a write fails and the connection is not aborted + (GH-23423). (Sjoerd Langkemper) + - FPM: . Use recommended TYPE, HELP order for OpenMetrics metadata. (Marcel Hernandez) diff --git a/sapi/embed/php_embed.c b/sapi/embed/php_embed.c index 3be8db4cc014..a69d22a3069a 100644 --- a/sapi/embed/php_embed.c +++ b/sapi/embed/php_embed.c @@ -75,6 +75,7 @@ static size_t php_embed_ub_write(const char *str, size_t str_length) ret = php_embed_single_write(ptr, remaining); if (!ret) { php_handle_aborted_connection(); + return str_length - remaining; } ptr += ret; remaining -= ret; diff --git a/tests/unit/Makefile b/tests/unit/Makefile index 778a03328238..0f0e0cca0639 100644 --- a/tests/unit/Makefile +++ b/tests/unit/Makefile @@ -3,11 +3,13 @@ CFLAGS = -g -Wall -I../../ -I../../Zend -I../../main -I../../TSRM -I. -I.. COMMON_LDFLAGS = ../../.libs/libphp.a -lcmocka -lpthread -lm -ldl -lresolv -lutil # Update paths in .github/workflows/unit-tests.yml when adding new test to make it run in PR when such file changes -TESTS = main/test_network main/test_io_copy +TESTS = main/test_network main/test_io_copy sapi/embed/test_embed_ub_write main/test_network_SRC = main/test_network.c main/test_network_LDFLAGS = $(COMMON_LDFLAGS) -Wl,--wrap=connect,--wrap=poll,--wrap=getsockopt,--wrap=gettimeofday main/test_io_copy_SRC = main/test_io_copy.c main/test_io_copy_LDFLAGS = $(COMMON_LDFLAGS) -Wl,--wrap=copy_file_range,--wrap=sendfile,--wrap=splice,--wrap=read,--wrap=write,--wrap=poll,--wrap=setsockopt +sapi/embed/test_embed_ub_write_SRC = sapi/embed/test_embed_ub_write.c +sapi/embed/test_embed_ub_write_LDFLAGS = $(COMMON_LDFLAGS) -Wl,--wrap=write # Build all tests diff --git a/tests/unit/sapi/embed/test_embed_ub_write.c b/tests/unit/sapi/embed/test_embed_ub_write.c new file mode 100644 index 000000000000..1cabc4a54732 --- /dev/null +++ b/tests/unit/sapi/embed/test_embed_ub_write.c @@ -0,0 +1,100 @@ +#include "php.h" +#include "sapi/embed/php_embed.h" +#include +#include + +#ifdef PHP_WRITE_STDOUT +ssize_t __wrap_write(int fd, const void *buf, size_t count) +{ + function_called(); + check_expected(fd); + check_expected(count); + return mock_type(ssize_t); +} + +struct ub_write_state { + bool ignore_user_abort; + short connection_status; +}; + +static int setup_ub_write(void **state) +{ + struct ub_write_state *saved = malloc(sizeof(*saved)); + + assert_non_null(saved); + saved->ignore_user_abort = PG(ignore_user_abort); + saved->connection_status = PG(connection_status); + + /* Keep php_handle_aborted_connection() from bailing out on failure. */ + PG(ignore_user_abort) = true; + PG(connection_status) = PHP_CONNECTION_NORMAL; + + *state = saved; + return 0; +} + +static int teardown_ub_write(void **state) +{ + struct ub_write_state *saved = *state; + + PG(ignore_user_abort) = saved->ignore_user_abort; + PG(connection_status) = saved->connection_status; + free(saved); + return 0; +} + +/* A failed write without progress must not retry forever. */ +static void test_ub_write_error_without_progress(void **state) +{ + expect_function_call(__wrap_write); + expect_value(__wrap_write, fd, STDOUT_FILENO); + expect_value(__wrap_write, count, 16); + will_return(__wrap_write, 0); + + assert_int_equal(php_embed_module.ub_write("0123456789abcdef", 16), 0); + assert_int_equal(PG(connection_status), PHP_CONNECTION_ABORTED); +} + +/* After a partial write, a failure must return the number of bytes written. */ +static void test_ub_write_partial_then_error(void **state) +{ + expect_function_calls(__wrap_write, 2); + expect_value_count(__wrap_write, fd, STDOUT_FILENO, 2); + expect_value(__wrap_write, count, 10); + expect_value(__wrap_write, count, 6); + will_return(__wrap_write, 4); + will_return(__wrap_write, 0); + + assert_int_equal(php_embed_module.ub_write("0123456789", 10), 4); + assert_int_equal(PG(connection_status), PHP_CONNECTION_ABORTED); +} + +/* Partial writes must continue until the whole buffer is written. */ +static void test_ub_write_partial_writes(void **state) +{ + expect_function_calls(__wrap_write, 2); + expect_value_count(__wrap_write, fd, STDOUT_FILENO, 2); + expect_value(__wrap_write, count, 10); + expect_value(__wrap_write, count, 6); + will_return(__wrap_write, 4); + will_return(__wrap_write, 6); + + assert_int_equal(php_embed_module.ub_write("0123456789", 10), 10); + assert_int_equal(PG(connection_status), PHP_CONNECTION_NORMAL); +} +#endif + +int main(void) +{ +#ifdef PHP_WRITE_STDOUT + const struct CMUnitTest tests[] = { + cmocka_unit_test_setup_teardown(test_ub_write_error_without_progress, setup_ub_write, teardown_ub_write), + cmocka_unit_test_setup_teardown(test_ub_write_partial_then_error, setup_ub_write, teardown_ub_write), + cmocka_unit_test_setup_teardown(test_ub_write_partial_writes, setup_ub_write, teardown_ub_write), + }; + + return cmocka_run_group_tests(tests, NULL, NULL); +#else + return 0; +#endif +}