Assertion `zval_get_type(&(*(zptr))) == 6 && "Concat should return string"' failed

[Changochen]

Description

The following code:

<?php
class A
{
    public string $prop = "";

}
class B
{
    public function __toString()
    {
        global $a;
        $a = $a = "";
        $a->p = "";
        return "";

    }

}
$a = new A();
$a = $a->prop .= new B();

?>

Resulted in this output:

php-src/Zend/zend_execute.c:1582: zend_binary_assign_op_typed_prop: Assertion `zval_get_type(&(*(zptr))) == 6 && "Concat should return string"' failed.

With asan, the output is:

==5503==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000001 (pc 0x555555ef095f bp 0x7fffffff9b00 sp 0x7fffffff9ad0 T0)
==5503==The signal is caused by a WRITE memory access.
==5503==Hint: address points to the zero page.
    #0 0x555555ef095e in zend_gc_addref /php-src/Zend/zend_types.h:1210
    #1 0x555555ef095e in ZEND_ASSIGN_OBJ_OP_SPEC_CV_CONST_HANDLER /php-src/Zend/zend_vm_execute.h:40931
    #2 0x555555f5c7cf in execute_ex /php-src/Zend/zend_vm_execute.h:60234
    #3 0x555555f91637 in zend_execute /php-src/Zend/zend_vm_execute.h:61087
...)

It seems a use-after-free bug.

PHP Version

PHP 8.3.0-dev

Operating System

No response

[nielsdos]

Looks like a variant of #10169. And therefore is related to PR #10546.

[iluuu1994]

I think this is related to #10179, rather. Unfortunately that fix did not cover this problem.

[iluuu1994]

Taking a quick look, it seems we'd need to increase the refcount of the object (that's not currently passed, refs might need special handling) in zend_binary_assign_op_typed_prop. As usual, this is unfortunate because it's not usually necessary. A VAR cannot be released, neither can a CV because it's not possible to access other scopes. The only exception are globals...

Edit: Ugh. Furthermore releasing the object in zend_binary_assign_op_typed_prop would once again cause issues with ZVAL_COPY(EX_VAR(opline->result.var), zptr); in the VM. This is a mess...

[nielsdos]

There needs to be a fundamental change to fix this in all cases, preferably in a uniform way. We could try patching it up ad-hoc for 8.1 and 8.2 and fix it better (in a different way) with a small BC break in 8.3 by delaying destructors uniformly until after the copies.

Edit: edited for clarity