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 #23423

Author: Sjord

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

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)

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;

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

tests/unit/sapi/embed/test_embed_ub_write.c

@@ -0,0 +1,100 @@
+#include "php.h"
+#include "sapi/embed/php_embed.h"
+#include <unistd.h>
+#include <cmocka.h>
+
+#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
+}