Assertion failure Zend/zend_exceptions.c:197

[YuanchengJiang]

Description

The following code:

<?php
if (defined("pass3")) {
} else {
function errorHandler($errorNumber, $errorMessage, $fileName, $lineNumber) {
define("pass3", 1);
include(__FILE__);
}
set_error_handler('errorHandler');
}
$resource = zend_test_create_throwing_resource();
try {ldap_delete_ext($fileName,$exception,$fileName);} catch (Exception $e) { echo($e); }

Resulted in this output:

php: /home/phpfuzz/WorkSpace/flowfusion/php-src/Zend/zend_exceptions.c:197: void zend_throw_exception_internal(zend_object *): Assertion `is_handle_exception_set() && "HANDLE_EXCEPTION not set?"' failed.
Aborted (core dumped)

PHP Version

nightly

Operating System

No response

[nielsdos]

Simplified test:

<?php
declare(strict_types=1);
function errorHandler($errorNumber, $errorMessage, $fileName, $lineNumber) {
    $resource = zend_test_create_throwing_resource();
    strlen(null);
}
set_error_handler('errorHandler');
echo $undef;

[nielsdos]

The assertion may be too conservative, as the HANDLE_EXCEPTION is set later on. But I'm not sure (yet).

diff --git a/Zend/zend_exceptions.c b/Zend/zend_exceptions.c
index 11b615c214a..0c3c6e8f743 100644
--- a/Zend/zend_exceptions.c
+++ b/Zend/zend_exceptions.c
@@ -193,7 +193,7 @@ ZEND_API ZEND_COLD void zend_throw_exception_internal(zend_object *exception) /*
 		zend_exception_set_previous(exception, EG(exception));
 		EG(exception) = exception;
 		if (previous) {
-			ZEND_ASSERT(is_handle_exception_set() && "HANDLE_EXCEPTION not set?");
+			//ZEND_ASSERT(is_handle_exception_set() && "HANDLE_EXCEPTION not set?");
 			return;
 		}
 	}

[iluuu1994]

This can be reproduced a bit simpler:

function test() {
    $resource = zend_test_create_throwing_resource();
    zend_test_create_throwing_resource();
}
test();

I think the issue is that zend_test_create_throwing_resource() will install the exception on the test() frame, unwind to main(), throw for the freed $resource variable, and verify that the exception op is set (which it isn't, as it was set in test() and only rethrown later.

Two possible solutions are:

• Switch to the previous stack frame after freeing the stack frames variables. This has some observable effects, namely for destructors. This might or might not matter.
• Adjust opline of the restored stack frame earlier. This seems like the less risky solution.

Details

diff --git a/Zend/zend_vm_def.h b/Zend/zend_vm_def.h
index 7e471b5acd8..fa950f81cfb 100644
--- a/Zend/zend_vm_def.h
+++ b/Zend/zend_vm_def.h
@@ -2925,6 +2925,9 @@ ZEND_VM_HOT_HELPER(zend_leave_helper, ANY, ANY)
 
 	if (EXPECTED((call_info & (ZEND_CALL_CODE|ZEND_CALL_TOP|ZEND_CALL_HAS_SYMBOL_TABLE|ZEND_CALL_FREE_EXTRA_ARGS|ZEND_CALL_ALLOCATED|ZEND_CALL_HAS_EXTRA_NAMED_PARAMS)) == 0)) {
 		EG(current_execute_data) = EX(prev_execute_data);
+		if (UNEXPECTED(EG(exception) != NULL)) {
+			zend_rethrow_exception(EG(current_execute_data));
+		}
 		i_free_compiled_variables(execute_data);
 
 #ifdef ZEND_PREFER_RELOAD
@@ -2939,7 +2942,6 @@ ZEND_VM_HOT_HELPER(zend_leave_helper, ANY, ANY)
 		execute_data = EX(prev_execute_data);
 
 		if (UNEXPECTED(EG(exception) != NULL)) {
-			zend_rethrow_exception(execute_data);
 			HANDLE_EXCEPTION_LEAVE();
 		}

WDYT?

[iluuu1994]

Looking a bit closer into this, ZEND_DO_ICALL is also susceptible to the same issue. It also restores the previous call frame and then freeing the popped stack frames variables without rethrowing first. I can't reproduce the issue though because the exception keeps the passed reference alive.

--INI--
zend.exception_ignore_args=1
--FILE--
function test() {
    \array_values(zend_test_create_throwing_resource());
}
test();

Maybe it's better to adjust the assertion, rather than adding additional checks everywhere.

[nielsdos]

I think the assertion should be adjusted because the end result works anyway

[iluuu1994]

Yeah I think I agree.

[iluuu1994]

@nielsdos I think it's ok to just remove these asserts. We don't have a good way to check whether we just switched to this frame and the exception opline will be installed later. Do you want to send a PR, since this was your original idea?

[nielsdos]

@iluuu1994 Well you did the more in-depth analysis, so it's at least a joint effort. I'll make a PR and add your C-o-b tag