Fix GH-15181: Disabled output handler is flushed again

[cmb69]

When an PHP_OUTPUT_HANDLER_FAILURE occurs, the output handler becomes disabled (i.e. the PHP_OUTPUT_HANDLER_DISABLED flag is set). However, there is no guard for disabled handlers in php_output_handler_op() what may cause serious issues (as reported, UB due to passing NULL as the 2nd argument of memcpy, because the handler's buffer has already been NULLed). Therefore, we add a respective guard for disabled handlers, and return PHP_OUTPUT_HANDLER_FAILURE right away.

---

Note that older PHP versions are affected by this (confirmed with PHP-8.3, but might be a long-standing issue), and although the behavior is obviously a bug, I'm still reluctant to deploy the fix to lower branches, given the complexities of PHP's output handling, and certain user expectations. For the given test scripts, unpatched PHP-8.3/master would output the fatal error and the respective stack backtrace.

[cmb69]

Note that there is a related issue regarding the output handling and PHP_OUTPUT_HANDLER_DISABLED:

php-src/main/output.c

Lines 1055 to 1061 in a578c27

	if (obh_cnt > 1) {
	zend_stack_apply_with_argument(&OG(handlers), ZEND_STACK_APPLY_TOPDOWN, php_output_stack_apply_op, &context);
	} else if ((active = zend_stack_top(&OG(handlers))) && (!((*active)->flags & PHP_OUTPUT_HANDLER_DISABLED))) {
	php_output_handler_op(*active, &context);
	} else {
	php_output_context_pass(&context);
	}

This code does not call php_output_handler_op() if the handler has been disabled, but rather falls through and calls php_output_context_pass(). This seems wrong to me; I think this guard should be removed, so that php_output_handler_op() is called even though the handler is disabled.

Removing the guard (on top of this PR) makes ob_015.phpt and the new test fail. However, it seems to me that ob_015.phpt should not produce the currently expected output. See:

php-src/tests/output/ob_015.phpt

Lines 5 to 11 in a578c27

	ob_start("str_rot13", 1);
	try {
	echo "foo\n";
	} catch (TypeError $e) {
	echo $e->getMessage(), "\n";
	}
	ob_end_flush();

After "foo\n" has been echoed, strrot13() is called, causing an exception to be thrown. This is caught, and the exception message is echoed, but that should not produce any output, because the handler has already been disabled. Instead, there should be a ob_end_clean(); at the beginning of the catch block, for that test to pass. Similar for the new test. I haven't checked any tests outside of tests/output/.

Anyhow, the failing CIs are unrelated to this PR.

PS:

full patch for this issue (barring any tests outside of tests/output/)

 main/output.c             | 2 +-
 tests/output/gh15181.phpt | 1 +
 tests/output/ob_015.phpt  | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/main/output.c b/main/output.c
index ef6be672d1..87f42c57bd 100644
--- a/main/output.c
+++ b/main/output.c
@@ -1058,7 +1058,7 @@ static inline void php_output_op(int op, const char *str, size_t len)
 
 		if (obh_cnt > 1) {
 			zend_stack_apply_with_argument(&OG(handlers), ZEND_STACK_APPLY_TOPDOWN, php_output_stack_apply_op, &context);
-		} else if ((active = zend_stack_top(&OG(handlers))) && (!((*active)->flags & PHP_OUTPUT_HANDLER_DISABLED))) {
+		} else if ((active = zend_stack_top(&OG(handlers)))) {
 			php_output_handler_op(*active, &context);
 		} else {
 			php_output_context_pass(&context);
diff --git a/tests/output/gh15181.phpt b/tests/output/gh15181.phpt
index 5fa5c272b3..7380d0c0e3 100644
--- a/tests/output/gh15181.phpt
+++ b/tests/output/gh15181.phpt
@@ -9,6 +9,7 @@
     ob_flush();
 } catch (Throwable) {}
 ob_flush();
+ob_end_clean();
 ?>
 ===DONE===
 --EXPECT--
diff --git a/tests/output/ob_015.phpt b/tests/output/ob_015.phpt
index 47c9b24be1..595ba04c42 100644
--- a/tests/output/ob_015.phpt
+++ b/tests/output/ob_015.phpt
@@ -6,9 +6,9 @@
 try {
     echo "foo\n";
 } catch (TypeError $e) {
+    ob_end_clean();
     echo $e->getMessage(), "\n";
 }
-ob_end_flush();
 ?>
 --EXPECT--
 foo

[nielsdos]

I think the patch is right for the original issue.

As for #15183 (comment):
I agree with your reasoning, this is very strange behaviour and I would expect the same thing you expected. However, I'm also a bit reluctant to change this at the last minute in 8.4 because of potential breaks. I remember in the thread https://externals.io/message/124506 that Jakub mentioned that the whole output buffering system needs to be more closely examined. So perhaps we should wait until we have a full understanding of all the intricacies for the second issue.

[Girgias]

@haszi I haven't had time to look at this yet as I'm on the train back from Glasgow, but considering you probably have the best knowledge of the intricacies of the output buffering handlers at the moment, having your opinion is valuable. :)

[bukka]

So I spent another hour an half looking through output.c and verifying this change and additional comment. Slowly starting to get how things work but need probably a bit more time and some debugging to have even better understanding.

Anyway this change looks good to me. It really doesn't make any sense to call php_output_handler_op when handler->buffer.data is NULL.

In terms of other comment, it looks also sensible but I'm not really sure about it. I need to think about it more. I agree that it's better to wait till we have more understanding as it might potentially break stuff.

[cmb69]

Thanks for checking, @bukka. I have applied this patch to master, and will provide a new PR for the other "fix" soon.

[bukka]

@cmb69 I did a bit more checking and realised that this is in some way a minor break. If someone has a handler with some side effect, they could potentially rely on it being called on each flush even though there was a failure (exception or false return). This is now being changed - it will no longer be called after the first failure. I think it makes sense to change but it might be potentially good to add a note to UPGRADING about it.

[cmb69]

@bukka, I can add a note to UPGRADING, but I wonder if (and how) that plays together with the deprecations regarding the output handler. Maybe @Girgias knows?

[Girgias]

Our output buffering is somewhat clunky.

I don't think output buffer handlers should have side effects (other than throwing exceptions) as this leads to very unintuitive behaviour. Both deprecations are related to this.

[bukka]

Yeah those side effects are not ideal but there might be users relying on them so we need to make them aware. That's what I mentioned in that thread - we need to first understand what all those side are and properly document any changes to it so the migration path is clear.