Fix GH-17941: Stack-use-after-return with lazy objects and hooks

[nielsdos]

zend_std_write_property() can return the variable pointer, but the code
was using a local variable, and so a pointer to a local variable could
be returned. Fix this by using the value pointer instead of the backup
value was written.

[nielsdos]

Actually, now the value between variable_ptr and value can be inconsistent. And it's a bit annoying that we can't "just" add a reference because the backupping of the value influences the semantics (i.e. is observable). I'm not going to deal with this.

[arnaud-lb]

I think the observable semantic change is right, because the current behavior is wrong.

Currently, $a->prop = rhs evaluates to either:

1. The value of rhs if the assignment was handled by __set or a set hook
2. The value of $a->prop after the assignment, in other cases

In the first case, when rhs is a CV, it evaluates to the value of rhs after the assignment. I'm not sure this is by design, but this is the behavior without lazy objects. When $a is a lazy object, this always evaluates to the value of rhs before the assignment, due to the backup.

Your change makes the behavior consistent, so I think it's right.

[nielsdos]

Okay, reopening for review then. I closed it because I didn't see an easy way to make the behaviour the same and didn't want to deal with this, but if the new behaviour is right then sure.

[iluuu1994]

I found another issue:

class C {
    public $prop {
        set {
            echo "set\n";
            $this->prop = $value * 2;
        }
    }
}

$rc = new ReflectionClass(C::class);

$obj = $rc->newLazyProxy(function () {
    echo "init\n";
    return new C;
});

function foo(C $c) {
    $c->prop = 1;
    var_dump($c->prop);
}

foo($obj);

set
init
set
int(4)

As you can see, the original 1 turns into 4. This happens because init() is called twice. That's because lazy init will repeat the assignment by calling zend_std_write_property(), in which zend_should_call_hook() will return true due to the parent frame calling the hook on the proxy, and the child calling it on the actual object. This could be solved in two ways:

• Continue calling set twice, but pass the original value (1 in this case) again, rather than the result from the last set call.
• Skip the set call for the second assignment, by considering proxy and object as equal somehow. This sounds like the better approach to me.

WDYT?

[nielsdos]

My poor head... Option 2 sounds best, I have yet to dig into the code that deals with proxies though. The first option would be bad because of observable side effects.

[arnaud-lb]

I also prefer the second approach, as it’s what would happen if the hook was a method.

It’s consistent with how lazy objects are specified: Interaction with the proxy executes the proxy’s code, and property accesses forward to the real instance. $this->prop = $value * 2 in the hook means “set the backing property”, so it makes sense to forward to the backing store of the real instance directly.

A third approach would have been to initialize before calling the hook, and to call the hook on the real instance. But the second approach is better.

Potentially this can be fixed separately. @nielsdos I can take care of it if you don’t want to.

[nielsdos]

A third approach would have been to initialize before calling the hook, and to call the hook on the real instance. But the second approach is better.

Yes, I still prefer the second approach as well, it's the most logical.

Potentially this can be fixed separately. @nielsdos I can take care of it if you don’t want to.

I agree this may be fixed separately. Feel free to give it a shot, I'm kinda busy right now 🙂

[iluuu1994]

Handling this separately sounds good to me! Feel free to merge this one, I'll create a new issue.

@arnaud-lb zend_should_call_hook() is normally skipped when SIMPLE_GET or READ is set, which should hopefully be most of the time. A call to zend_object_is_lazy() and then fetching the real object should not be too expensive. I can take care of this.

[arnaud-lb]

Thank you @nielsdos!