Fix lazy proxy calling magic methods twice #18039
Conversation
| @@ -970,6 +982,43 @@ static zend_always_inline bool property_uses_strict_types(void) { | |||
| && ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data)); | |||
| } | |||
|
|
|||
| static zend_always_inline zval *forward_write_to_lazy_object(zend_object *zobj, | |||
There was a problem hiding this comment.
I don't know if it's worth to always_inline this?
There was a problem hiding this comment.
Indeed, I will check if it makes a noticeable difference
| var_dump($obj->prop); | ||
|
|
||
| ?> | ||
| --EXPECTF-- |
There was a problem hiding this comment.
I don't understand why the __get function should get called before instantiating the object? I.e. why does "C::__get" appear before "init"?
There was a problem hiding this comment.
It’s how lazy objects are supposed to behave: we interact primarily with the proxy (we execute the proxy’s code), but any access to its state is forwarded to the real instance.
So here we execute the getter, which tries to access a property. This triggers initialization before the access is forwarded.
There was a problem hiding this comment.
Okay I see, not what I intuitively expected but it makes sense
There was a problem hiding this comment.
I suppose there's also an implicit assumption here that the initialized properties does not contain dynamic properties. If it does, the initializer would need to be called before __get, to ensure that the property is actually still does not exist after the initializer has been called.
| uint32_t guard_type = (type == BP_VAR_IS) && zobj->ce->__isset | ||
| ? IN_ISSET : IN_GET; | ||
| guard = zend_get_property_guard(zobj, name); | ||
| if (!((*guard) & guard_type)) { | ||
| (*guard) |= guard_type; | ||
| retval = zend_std_read_property(zobj, name, type, cache_slot, rv); | ||
| (*guard) &= ~guard_type; | ||
| return retval; | ||
| } |
There was a problem hiding this comment.
I think this can be slightly simplified:
| uint32_t guard_type = (type == BP_VAR_IS) && zobj->ce->__isset | |
| ? IN_ISSET : IN_GET; | |
| guard = zend_get_property_guard(zobj, name); | |
| if (!((*guard) & guard_type)) { | |
| (*guard) |= guard_type; | |
| retval = zend_std_read_property(zobj, name, type, cache_slot, rv); | |
| (*guard) &= ~guard_type; | |
| return retval; | |
| } | |
| uint32_t *instance_guard = zend_get_property_guard(zobj, name); | |
| uint32_t prev_guard = *instance_guard; | |
| *instance_guard = *guard; | |
| retval = zend_std_read_property(zobj, name, type, cache_slot, rv); | |
| *instance_guard = prev_guard; | |
| return retval; |
There was a problem hiding this comment.
Unfortunately this may remove guards from the instance, which may be observable. The instance is an independent object that may be accessed directly, and may have active guards.
There was a problem hiding this comment.
Ah, I missed that. This looks fine then!
| (*guard) |= IN_SET; | ||
| variable_ptr = zend_std_write_property(instance, name, &backup, cache_slot); | ||
| (*guard) &= ~IN_SET; | ||
| goto exit; |
There was a problem hiding this comment.
Same as above. Dropping the inner if would also allow moving the second write to an else, dropping the goto.
I'll leave it up to you to decide whether that is cleaner.
|
|
||
| if (UNEXPECTED(guard)) { | ||
| guard = zend_get_property_guard(zobj, name); | ||
| if (!((*guard) & IN_ISSET)) { |
There was a problem hiding this comment.
Should this not be IS_UNSET? Zend/tests/lazy_objects/gh18038-010.phpt looks wrong.
Fixes GH-18038.
Guard the underlying property, so that the forwarded operation accesses the property directly.