-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
Comments
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; |
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;
}
}
|
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 Two possible solutions are:
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? |
Looking a bit closer into this, --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. |
I think the assertion should be adjusted because the end result works anyway |
Yeah I think I agree. |
@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? |
@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 |
`zend_test_create_throwing_resource` sets the exception in the `test` call frame and unwinds to `main`. It then throws for the `resource` variable and verifies that the exception opline is set. However, it wasn't set then, it was set at the `test` call frame and rethrown later. The assertion is too conservative, but the end result is right, so drop the assertion.
`zend_test_create_throwing_resource` sets the exception in the `test` call frame and unwinds to `main`. It then throws for the `resource` variable and verifies that the exception opline is set. However, it wasn't set in `main`, it was set at the `test` call frame and rethrown later. The assertion is too conservative, but the end result is right, so drop the assertion. Co-authored-by: Ilija Tovilo <[email protected]>
* PHP-8.3: Fix GH-17408: Assertion failure Zend/zend_exceptions.c
* PHP-8.4: Fix GH-17408: Assertion failure Zend/zend_exceptions.c
Description
The following code:
Resulted in this output:
PHP Version
nightly
Operating System
No response
The text was updated successfully, but these errors were encountered: