Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Assertion failure Zend/zend_exceptions.c:197 #17408

Open
YuanchengJiang opened this issue Jan 9, 2025 · 6 comments
Open

Assertion failure Zend/zend_exceptions.c:197 #17408

YuanchengJiang opened this issue Jan 9, 2025 · 6 comments

Comments

@YuanchengJiang
Copy link

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
Copy link
Member

nielsdos commented Jan 9, 2025

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
Copy link
Member

nielsdos commented Jan 9, 2025

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;
 		}
 	}

@nielsdos nielsdos removed their assignment Jan 9, 2025
@iluuu1994
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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

@iluuu1994
Copy link
Member

Yeah I think I agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants