Fix volatile INDICECT with .=

[iluuu1994]

Neither STATIC_PROP nor VAR are susceptible. Static vars cannot be unset.
Unsetting vars only sets them to IS_UNDEF, including dynamic properties
(zend_hash_del_ind() in the ZEND_UNSET_VAR handler).

Fixes GH-15938

[iluuu1994]

Note: This line isn't even necessary to trigger the leak. $this->a .= prepares the index in the property table and stores the pointer in the VAR passed to ASSIGN_OP. unset($obj->a) will then remove the key and NULL value that were just created. The actual store will then occur in a dead zval.

[iluuu1994]

Hmm, I just realized that this changes behavior due to ZEND_TRY_BINARY_OBJECT_OPERATION(ZEND_CONCAT), which would now always receive a string, whereas previously it could have received arbitrary values, including array/objects/etc. I don't think there's a solution for this, unfortunately.

[arnaud-lb]

This would have been a nice solution. The optimizer is able to optimize-out the cast sometimes, and I see no perf regression on the symfony-demo benchmark.

An other case it doesn't handle is this:

<?php

class C {
    public string $a;
}

$obj = new C();

$v = new class {
    function __toString() {
        global $obj;
        $obj = null;
        return 'str';
    }
};

$obj->a = &$v;

[nielsdos]

This is a neat idea. I'm not sure though if we should be doing this, we're playing increasingly complex games to fix these kinds of issues, a generic "zval locking" mechanism (for the lack of a better term) that would cover as much cases as possible would be nice ig.

EDIT: To be clear, I like this idea and perhaps we should commit this; but I'm starting to doubt adding all these tiny things all over the place for specific cases is the way to go. One one hand we should fix as many as possible of these, on the other hand it's all subtle stuff and complexity increases.

[iluuu1994]

@nielsdos Yeah I agree it would be nice to have a global solution. Some spitballing:

Essentially, FETCH_{OBJ,DIM}_[R]W should set a flag on the given container until the assignment is done. When a locked object or array is freed/resized, we would need to throw to skip over the assignment, or make sure the memory persists. This is needed when user code can execute between fetch + assignment, which I believe are in two primary cases:

1. For the last fetched offset, when the assignment itself can have side-effects, either through .= or type coercion for typed properties.
2. For intermediate fetches, when emitting the "Creation of dynamic property" or "undefined array key" (RW) warnings.

Number 1. is relatively easy to fix. We can lock the current instructions container in ASSIGN_{OBJ,DIM}[_OP], perform the assignment, and then unlock it again. Number 2. seems similarly simple. When creating undefined offsets and then emitting the error, simply lock the container beforehand and unlock it after. 2. should have essentially no performance overhead, since it's only necessary when emitting the warning. The overhead of 1. is hard to estimate.

As for what happens when modifying the locked object, we have the two aforementioned approaches:

1. We could either throw an exception and make sure it cannot be caught until after the original assignment has unwound. Otherwise, catching the exception would continue normal control flow and still perform the assignment on the released memory.
2. We could probably simply increase the refcount of the object/array. For both objects and arrays, this will make sure the object won't be released, and as such the zval will stay valid. For arrays specifically, we'll also make sure that it cannot be modified, as any addition/modification would cause it to be separated. One additional caveat for objects is that UNSET of dynamic properties would need to be prevented to avoid Zend/tests/gh15938_001.phpt, or we would also increase the refcount of the properties table. This seems overall like the simpler approach.

I haven't done any testing, but it seems like this should work. It's late, so it might also not make sense. 😄

Edit: And what I forgot. For the undefined offset warnings, we would need to terminate when unlocking if the container goes away.

[iluuu1994]

It also just occurred to me that references are potentially also subject to this problem. And of course they are. :(

class C {
    public mixed $prop1;
    public ?string $prop2;

    public function __toString() {
        unset($this->prop1);
        unset($this->prop2);
        return 'bar';
    }
}

function test() {
    $c = new C();
    $c->prop1 = 'foo';
    $c->prop1 = &$c->prop2;
    $c->prop1 = $c;
    var_dump($c);
}

test();

That said, this can probably work the same way. Increase the refcount before the type coercion, and release after the assignment.

[arnaud-lb]

This is similar in some ways to the solutions I briefly suggested in #15938.

The second approach seems simpler indeed, at least for arrays and maybe references.

For objects, since implementations of get_property_ptr_ptr or read_property(..., &rv) are free to return any address outside of the zend_object and the properties ht (e.g. internal classes or lazy objects might do that), we can not assume that increasing the refcount of the properties ht is enough. We may need the help of the object handlers: Give the responsibility of locking to get_property_ptr_ptr and read_property(..., &rv), and add a new unlock handler. Or can we replace usages of get_property_ptr_ptr/read_property(..., &rv) by a new one: write_property_op(obj, op_func, value)? This would let us handle this entirely in write_property_op for ASSIGN_OBJ_{OP,REF}, in a similar way to how write_property handles this.

ASSIGN_OBJ with a primed cache slot and valid offset may only need to addref/delref the object itself around zend_assign_to_typed_prop. However, it needs to check whether the property is undef before writing to it, following a coercion.