PHP hanging infinitly at 100% cpu when check php syntaxe of a valid file

[pierre-dominique]

Description

The following code:

<?php
class klass
{
    public function method(int $int, bool $bool=false) : int
    {
        while($int >= 0 && !$function1)
            $int--;
        if($bool)
            while($int >= 0 && $function1)
                $int--;
        return $int;
    }
}

Resulted in this output:

php8.1 -l test.php                       

But I expected this output instead:

php8.1 -l test.php   
No syntax errors detected in test.php                    

hanging infinitely, can't be canceled, sigkill 6 needed.
use 100% off one cpu

When opcache is disable it's OK
When jit in opcache is disable it's OK
When jit is opcache.jit = 1255 it's OK
when jit is opcache.jit = 1205 its' KO

The zero from 1205 look suspicious

Here the opcache conf

; configuration for php ZendOpcache module
zend_extension=opcache.so
opcache.enable=1
opcache.enable_cli=1
opcache.use_cwd=1
opcache.max_accelerated_files=20000
;2000 par instance
opcache.enable_cli=1
opcache.load_comments=0
opcache.save_comments=0
opcache.memory_consumption=1500
;150M par instance
opcache.dups_fix=0
opcache.enable_file_override=1
opcache.optimization_level=0x7FFEBFFF
;dev
opcache.validate_timestamps=1
opcache.revalidate_freq=0
;prod
;opcache.validate_timestamps=1
;opcache.revalidate_freq=5
;opcache.consistency_checks=0
;opcache.file_update_protection=1
;opcache.jit = disable ; desactive [OK]
opcache.jit = 1205 ; init terrible et perfs correctes [OK]
opcache.jit_buffer_size = 512M

here the output of opcache

$_main:
     ; (lines=1, args=0, vars=0, tmps=0)
     ; (after pass 1)
     ; test.php:1-14
     ; return  [] RANGE[0..0]
0000 RETURN int(1)

klass::method:
     ; (lines=20, args=2, vars=3, tmps=5)
     ; (after pass 1)
     ; test.php:4-12
     ; return  [] RANGE[0..0]
0000 CV0($int) = RECV 1
0001 CV1($bool) = RECV_INIT 2 bool(false)
0002 JMP 0004
0003 PRE_DEC CV0($int)
0004 T4 = IS_SMALLER_OR_EQUAL int(0) CV0($int)
0005 T4 = JMPZ_EX T4 0008
0006 T5 = BOOL_NOT CV2($function1)
0007 T4 = BOOL T5
0008 JMPNZ T4 0003
0009 JMPZ CV1($bool) 0016
0010 JMP 0012
0011 PRE_DEC CV0($int)
0012 T7 = IS_SMALLER_OR_EQUAL int(0) CV0($int)
0013 T7 = JMPZ_EX T7 0015
0014 T7 = BOOL CV2($function1)
0015 JMPNZ T7 0011
0016 VERIFY_RETURN_TYPE CV0($int)
0017 RETURN CV0($int)
0018 VERIFY_RETURN_TYPE
0019 RETURN null

=>hanging here

To reproduce with docker, php 8.0.x to 8.1.x

docker run -it php:8.0.0-zts-buster sh

In the console

rm -f x.php ; echo "<?php\nclass klass\n{\npublic function method(int \$int, bool \$bool=false) : int\n{\nwhile(\$int >= 0 && !\$function1)\n\$int--;\nif(\$bool)\nwhile(\$int >= 0 && \$function1)\n\$int--;\nreturn \$int;\n}\n}" > x.php ; php -l x.php
php -d zend_extension=opcache.so -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.use_cwd=1 -d opcache.max_accelerated_files=20000 -d opcache.enable_cli=1 -d opcache.load_comments=0 -d opcache.save_comments=0 -d opcache.memory_consumption=1500 -d opcache.dups_fix=0 -d opcache.enable_file_override=1 -d opcache.optimization_level=0x7FFEBFFF -d opcache.validate_timestamps=1 -d opcache.revalidate_freq=0 -d opcache.jit=1205 -d opcache.jit_buffer_size=512M -l x.php

PHP Version

PHP 8.0.x and 8.1.x

Operating System

Debian

[Geompse]

Confirmed, reproduced in : PHP 8.0.0, PHP 8.1.0, PHP 8.1.4, PHP 8.1.5, PHP 8.1.6, PHP 8.1.7

docker run -it php:8.0.0-zts-buster sh
docker run -it php:8.1.0-zts-bullseye sh
docker run -it php:8.1.4-zts-bullseye sh
docker run -it php:8.1.5-zts-bullseye sh
docker run -it php:8.1.6-zts-bullseye sh
docker run -it php:8.1.7-zts-bullseye sh

The JIT configuration seems valid.

[cmb69]

It hangs during JIT compilation in zend_jit_add_hint, where dst changes between 8 and 9 but the loop is never terminated.

[Geompse]

To me, it is always a good practice to have exit conditions on while loops. If dst never changes, or changes to a previous value, the loop never exits. The "true" fix might be elsewhere, but a simple counter should permanently prevent hanging.

Something like this (would be time-consuming) :

+       int counter = 0;
	while (1) {
		if (intervals[dst]->hint) {
			if (intervals[dst]->hint->range.start < intervals[src]->range.start) {
				int tmp = src;
				src = intervals[dst]->hint->ssa_var;
				dst = tmp;
			} else {
				dst = intervals[dst]->hint->ssa_var;
			}
		} else {
			if (dst != src) {
				intervals[dst]->hint = intervals[src];
			}
			return;
		}
+               counter++;
+               if(counter > 2000000000)
+                   return; // or throw
	}

A more memory-consuming way would be to check if the new value of dst has already been used.

[arnaud-lb]

From my vague understanding, zend_jit_add_hint is used to add register hints as described in https://www.usenix.org/legacy/events/vee05/full_papers/p132-wimmer.pdf. This is used to coalesce lifetime intervals without violating SSA. I understand that intervals[dst]->hint = intervals[src] basically means that the SSA var dst can use the same register as src.

We call zend_jit_add_hint(dst, src) when there is an assignment from src to dst. I think that cycles in assignments can produce cycles in hints, because we will end up calling zend_jit_add_hint(dst, src) and zend_jit_add_hint(src, dst).

I think that the purpose of

php-src/ext/opcache/jit/zend_jit.c

Line 1654 in 0429159

if (intervals[dst]->range.start < intervals[src]->range.start) {

and

php-src/ext/opcache/jit/zend_jit.c

Line 1661 in 0429159

if (intervals[dst]->hint->range.start < intervals[src]->range.start) {

is to prevent these cycles by only creating hints that point to previous op lines.

However, in the case of $int--, we have two ssa vars, that are part of an assignment cycle (because of phis), and both have a lifetime that starts in the same op line, so intervals[dst]->range.start < intervals[src]->range.start is always false, and this allows to create a hint cycle.

We could fallback to comparing ssa vars when range.start are equal.

I'm not sure that this is correct, but this fixes the problem for me:

diff --git a/ext/opcache/jit/zend_jit.c b/ext/opcache/jit/zend_jit.c
index 8ed75b7b7c..9cda453992 100644
--- a/ext/opcache/jit/zend_jit.c
+++ b/ext/opcache/jit/zend_jit.c
@@ -1651,14 +1651,14 @@ static void zend_jit_compute_loop_body(zend_ssa *ssa, int header, int n, zend_bi
 
 static void zend_jit_add_hint(zend_lifetime_interval **intervals, int dst, int src)
 {
-	if (intervals[dst]->range.start < intervals[src]->range.start) {
+	if (intervals[dst]->range.start < intervals[src]->range.start || (intervals[dst]->range.start == intervals[src]->range.start && dst < src)) {
 		int tmp = src;
 		src = dst;
 		dst = tmp;
 	}
 	while (1) {
 		if (intervals[dst]->hint) {
-			if (intervals[dst]->hint->range.start < intervals[src]->range.start) {
+			if (intervals[dst]->hint->range.start < intervals[src]->range.start || (intervals[dst]->hint->range.start == intervals[src]->range.start && intervals[dst]->hint->ssa_var < src)) {
 				int tmp = src;
 				src = intervals[dst]->hint->ssa_var;
 				dst = tmp;

[cmb69]

@arnaud-lb, that sounds plausible. I suggest to submit a PR, and request a review from @dstogov.