Fix some increment/decrement bugs

[Girgias]

So the first commit probably should be backported as that's a pre-existing issue. (Also this issue exists for nearly every other case where we have ZVAL_UNDEFINED_OP1()/ZVAL_UNDEFINED_OP2() without a check for EG(exception)).
Spun-out into #11850

Thanks to @arnaud-lb for helping me pinpoint the issues as they were kinda tricky.

[dstogov]

I think this is wrong. We shouldn't change the property. In worst case this may be a typed property that can't be NULL. There are few similar places in the patch.

[Girgias]

@arnaud-lb gave me a solution that I am going to try out which should hopefully fix those in a sensible manner.

[Girgias]

@dstogov I'm having a weird bug with the function JIT with test Zend/tests/in-de-crement/incdec_undefined_vars_exception.phpt when running the following test command:
make -j22 && make test TEST_PHP_ARGS="--asan -q -j22 -d opcache.enable_cli=1 -d opcache.jit_buffer_size=16M -d opcache.jit=function -d opcache.enable=1 -d opcache.enable_cli=1 -d opcache.file_update_protection=0 -x" TESTS="Zend/tests/in-de-crement"

The output is:

Undefined variable $x
int(1)
Decrement on type null has no effect, this will change in the next major version of PHP
NULL
Undefined variable $x
NULL

Where it seems neither the exceptions are being thrown, but also one of the operations just disappears, @nielsdos had a quick look, and they could also reproduce it and seem to think there is an existing bug in the JIT.

[dstogov]

Function JIT does exactly the same that VM does. The output of Zend/tests/in-de-crement/incdec_undefined_vars_exception.phpt in php master is the same with and without JIT.

So if you fixed a bug in VM, this should be reflected in JIT (may be some of JIT helpers or JIT code generator itself).

Also, I can't follow your trick in error handler if ($count++ % 2 == 0) { and its sub-sequences.

[Girgias]

Function JIT does exactly the same that VM does. The output of Zend/tests/in-de-crement/incdec_undefined_vars_exception.phpt in php master is the same with and without JIT.

So if you fixed a bug in VM, this should be reflected in JIT (may be some of JIT helpers or JIT code generator itself).

Also, I can't follow your trick in error handler if ($count++ % 2 == 0) { and its sub-sequences.

The trick is to be able to var_dump() the undefined variable without it throwing an exception again

[dstogov]

Using if (!isset($x)) echo("UNDEF\n"); else var_dump($x) would be more robust.

[Girgias]

Using if (!isset($x)) echo("UNDEF\n"); else var_dump($x) would be more robust.

Okay, so doing this now actually produces all the operands and keeps the value undef for the -- op, but not the ++ op.

Seems using a static variable in an error handler confuses the JIT, but that's an unrelated issue.

[dstogov]

PHP master branch.

$ cat x.php 
<?php
set_error_handler(function($severity, $m) {
   throw new Exception($m, $severity);
});

unset($x);
try {
    $x++;
} catch (\Exception $e) {
    echo "1:" . $e->getMessage(), PHP_EOL;
    if (!isset($x)) echo "undef\n"; else var_dump($x);
}
unset($x);
try {
    $x--;
} catch (\Exception $e) {
    echo "2:" . $e->getMessage(), PHP_EOL;
    if (!isset($x)) echo "undef\n"; else var_dump($x);
}
unset($x);
try {
    ++$x;
} catch (\Exception $e) {
    echo "3:" . $e->getMessage(), PHP_EOL;
    if (!isset($x)) echo "undef\n"; else var_dump($x);
}
unset($x);
try {
    --$x;
} catch (\Exception $e) {
    echo "4:" . $e->getMessage(), PHP_EOL;
    if (!isset($x)) echo "undef\n"; else var_dump($x);
}
unset($x);
?>
$ sapi/cli/php -d opcache.jit=0 x.php 
1:Undefined variable $x
int(1)
2:Undefined variable $x
undef
3:Undefined variable $x
int(1)
4:Undefined variable $x
undef
$ sapi/cli/php -d opcache.jit=1205 x.php 
1:Undefined variable $x
int(1)
2:Undefined variable $x
undef
3:Undefined variable $x
int(1)
4:Undefined variable $x
undef

[iluuu1994]

I think we would semantically be more consistent to assign to assign to result before emitting the warning, similar to what we do for destructors. This way, nothing can change the result mid-operation. Unfortunately, passing the result pointer to the increment/decrement function could mean changing quite a few handlers in the VM. I'm not sure if that is acceptable.

Another option would be to fetch the result pointer through current_execute_data similar to this code here:

php-src/Zend/zend_object_handlers.c

Lines 852 to 860 in 5da08df

	if (execute_data
	&& EX(func)
	&& ZEND_USER_CODE(EX(func)->common.type)
	&& EX(opline)
	&& EX(opline)->opcode == ZEND_ASSIGN_OBJ
	&& EX(opline)->result_type) {
	ZVAL_COPY_DEREF(EX_VAR(EX(opline)->result.var), variable_ptr);
	variable_ptr = NULL;
	}

Given that this warning is temporary and shouldn't add an additional overhead to the happy path, that might be an acceptable option.

[dstogov]

@Girgias This HANDLE_EXCEPTION() was introduced by your previous patch. Note that it returns from the handler without execution FREE_OP1_VAR_PTR(). This may lead to memory leak.

CC @iluuu1994

[dstogov]

This is the PoC. It doesn't leak in PHP-8.2.

<?php
set_error_handler(function($severity, $m) {
	if (strpos($m, 'Indirect modification of overloaded element') === 0) return;
	throw new Exception($m, $severity);
});
class Foo implements ArrayAccess {
    function offsetGet($index): mixed {
        return range(1, 5);
    }
    function offsetSet($index, $newval): void {
    }
    function offsetExists($index): bool {
        return true;
    }
    function offsetUnset($index): void {
    }
}
$foo = new Foo;
try {
	$foo[0]++;
} catch (Throwable $ex) {
	echo $ex->getMessage() . "\n";
}

[Girgias]

Right >_> thanks for catching this.