From ee0143887dcdf971b27a5193e8f140c150dc268c Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+ndossche@users.noreply.github.com> Date: Sun, 2 Nov 2025 00:07:47 +0100 Subject: [PATCH 1/2] Fix GH-20352: UAF in php_output_handler_free via re-entrant ob_start() during error deactivation The problem is that the code is doing `php_output_handler_free` in a loop on the output stack, but prior to freeing the pointer on the stack in `php_output_handler_free` it calls `php_output_handler_dtor` which can run user code that reallocates the stack, resulting in a dangling pointer freed by php_output_handler_free. Furthermore, OG(active) is set when creating a new output handler, but the loop is supposed to clean up all handlers, so OG(active) must be reset as well. Closes GH-20356. --- NEWS | 2 ++ main/output.c | 13 +++++++++---- tests/output/gh20352.phpt | 24 ++++++++++++++++++++++++ 3 files changed, 35 insertions(+), 4 deletions(-) create mode 100644 tests/output/gh20352.phpt diff --git a/NEWS b/NEWS index d7fd92cf997fd..2c6bab631e615 100644 --- a/NEWS +++ b/NEWS @@ -8,6 +8,8 @@ PHP NEWS . Fixed bug GH-20695 (Assertion failure in normalize_value() when parsing malformed INI input via parse_ini_string()). (ndossche) . Fixed bug GH-20714 (Uncatchable exception thrown in generator). (ilutov) + . Fixed bug GH-20352 (UAF in php_output_handler_free via re-entrant + ob_start() during error deactivation). (ndossche) - Bz2: . Fixed bug GH-20620 (bzcompress overflow on large source size). diff --git a/main/output.c b/main/output.c index c2cb0ceab0112..a650f116c7e1c 100644 --- a/main/output.c +++ b/main/output.c @@ -188,8 +188,12 @@ PHPAPI void php_output_deactivate(void) /* release all output handlers */ if (OG(handlers).elements) { while ((handler = zend_stack_top(&OG(handlers)))) { - php_output_handler_free(handler); zend_stack_del_top(&OG(handlers)); + /* It's possible to start a new output handler and mark it as active, + * however this loop will destroy all active handlers. */ + OG(active) = NULL; + ZEND_ASSERT(OG(running) == NULL && "output is deactivated therefore running should stay NULL"); + php_output_handler_free(handler); } } zend_stack_destroy(&OG(handlers)); @@ -719,10 +723,11 @@ PHPAPI void php_output_handler_dtor(php_output_handler *handler) * Destroy and free an output handler */ PHPAPI void php_output_handler_free(php_output_handler **h) { - if (*h) { - php_output_handler_dtor(*h); - efree(*h); + php_output_handler *handler = *h; + if (handler) { *h = NULL; + php_output_handler_dtor(handler); + efree(handler); } } /* }}} */ diff --git a/tests/output/gh20352.phpt b/tests/output/gh20352.phpt new file mode 100644 index 0000000000000..16be0b920e80f --- /dev/null +++ b/tests/output/gh20352.phpt @@ -0,0 +1,24 @@ +--TEST-- +GH-20352 (UAF in php_output_handler_free via re-entrant ob_start() during error deactivation) +--FILE-- + +--EXPECTF-- +Fatal error: ob_start(): Cannot use output buffering in output buffering display handlers in %s on line %d From 4315c3a5aa1380ebf874daa9a2629e83ad4afa46 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+ndossche@users.noreply.github.com> Date: Fri, 19 Dec 2025 12:11:17 -0800 Subject: [PATCH 2/2] standard: Remove nonsensical dtor of return value in fread() (#20739) It was never set to a string at this point, so why dtor it? --- ext/standard/file.c | 1 - 1 file changed, 1 deletion(-) diff --git a/ext/standard/file.c b/ext/standard/file.c index fdeabd1872d20..4168cd0f84b79 100644 --- a/ext/standard/file.c +++ b/ext/standard/file.c @@ -1555,7 +1555,6 @@ PHPAPI PHP_FUNCTION(fread) str = php_stream_read_to_str(stream, len); if (!str) { - zval_ptr_dtor_str(return_value); RETURN_FALSE; }