Skip to content

Assertion failure with tracing JIT #9011

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

Closed
cmb69 opened this issue Jul 14, 2022 · 2 comments · Fixed by #17042
Closed

Assertion failure with tracing JIT #9011

cmb69 opened this issue Jul 14, 2022 · 2 comments · Fixed by #17042

Comments

@cmb69
Copy link
Member

cmb69 commented Jul 14, 2022

Description

The following code (script provided by @zeriyoshi):

<?php

$foo = [];
$foo[] = new \Exception(); /* Native interface implemented Native instance */
$foo[] = new class () implements \Stringable /* Native interface implemented User instance */
{
    public function __toString(): string
    {
        return "bar";
    }
};

foreach ($foo as $baz) {
    for ($i = 0; $i < 64; $i++) {
        $baz->__toString();
    }
}

Resulted in this output:

Assertion failed: ((execute_data)->opline) >= ((execute_data)->func)->op_array.opcodes && ((execute_data)->opline) < ((execute_data)->func)->op_array.opcodes + ((execute_data)->func)->op_array.last, file C:\php-sdk\phpdev\vs16\x64\php-src-8.0\ext\opcache\jit/zend_jit_trace.c, line 7679

ZEND_ASSERT(EX(opline) >= EX(func)->op_array.opcodes &&
EX(opline) < EX(func)->op_array.opcodes + EX(func)->op_array.last);

But I expected this output instead:

PHP Version

= PHP-8.0

Operating System

Windows, maybe others as well

@nielsdos
Copy link
Member

nielsdos commented Nov 30, 2024

I've debugged this for a while and can only reproduce this on Windows. It seemed related to the handling of side exits with internal functions.
I noticed side exit generation code here:

if ((!func || zend_jit_may_be_modified(func, op_array))
&& trace
&& trace->op == ZEND_JIT_TRACE_INIT_CALL
&& trace->func
#ifdef _WIN32
&& trace->func->type != ZEND_INTERNAL_FUNCTION
#endif

The last part of the check looked suspicious to me. Why do we skip internal functions on Windows? I would expect the opposite: because of ASLR we do have to make a guard that checks if the function address is unchanged. Therefore, we must execute this code for internal functions as well on Windows to get the potential side exit.

The reason this fixes the issue here is because in the trace we initially inferred the function to execute being Exception::__toString, but in the second outer iteration we must execute a userland function. Because the initial trace used an internal function, the guard is not generated and so the generated code will use the same function again instead of the userland function, leading to the assertion failure.

Patch made on branch PHP-8.4. (On master, more places may need changes, I could do this if this is the right approach)

diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c
index e4b68d23520..a1612948782 100644
--- a/ext/opcache/jit/zend_jit_ir.c
+++ b/ext/opcache/jit/zend_jit_ir.c
@@ -9006,9 +9006,6 @@ static int zend_jit_init_method_call(zend_jit_ctx         *jit,
 	 && trace
 	 && trace->op == ZEND_JIT_TRACE_INIT_CALL
 	 && trace->func
-#ifdef _WIN32
-	 && trace->func->type != ZEND_INTERNAL_FUNCTION
-#endif
 	) {
 		int32_t exit_point;
 		const void *exit_addr;

I thought first, an alternative solution is maybe to check if the function type is still the same, but that would not be enough probably because of ASLR.

I could be wrong. cc @dstogov
Also, this would need to be fixed in 8.2 too, but I wanted to show the patch for IR JIT because IR JIT is easier to understand.

dstogov added a commit to dstogov/php-src that referenced this issue Dec 4, 2024
@dstogov
Copy link
Member

dstogov commented Dec 4, 2024

@nielsdos thanks for analyses.
Handling of functions guards on Windows was really inconsistent.

The comparison of zend_internal_functions on Windows may be inaccurate because of ASLR.
I created a PR for PHP-8.4 that fixes this comparing the function handlers.

I may back-port the fix to PHP-8.2/8.3 later.

@cmb69 cmb69 linked a pull request Dec 4, 2024 that will close this issue
@dstogov dstogov closed this as completed in 5ab2c02 Dec 4, 2024
dstogov added a commit that referenced this issue Dec 4, 2024
* PHP-8.4:
  Fix GH-9011: Assertion failure with tracing JIT (#17042)
dstogov added a commit to dstogov/php-src that referenced this issue Dec 5, 2024
dstogov added a commit that referenced this issue Dec 5, 2024
* Backport fix for GH-9011

* Fix build
dstogov added a commit that referenced this issue Dec 5, 2024
* PHP-8.2:
  Backport fix for GH-9011 (#17052)
dstogov added a commit that referenced this issue Dec 5, 2024
* PHP-8.3:
  Backport fix for GH-9011 (#17052)
dstogov added a commit that referenced this issue Dec 5, 2024
* PHP-8.4:
  Backport fix for GH-9011 (#17052)
charmitro pushed a commit to wasix-org/php that referenced this issue Mar 13, 2025
* Backport fix for phpGH-9011

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

Successfully merging a pull request may close this issue.

3 participants