Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ReflectionProperty::getRawValue(), setRawValue(), setRawValueWithoutLazyInitialization() for properties overridden with hooks #17714

Closed
wants to merge 4 commits into from
+238 −42
Closed

Fix ReflectionProperty::getRawValue(), setRawValue(), setRawValueWithoutLazyInitialization() for properties overridden with hooks #17714

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8b74af6
Fetch the effective (overridden) property info before checking hooks …
arnaud-lb Feb 6, 2025
f57304e
Update test
arnaud-lb Feb 6, 2025
4830b52
Update error message
arnaud-lb Feb 6, 2025
cb62612
Fix test
arnaud-lb Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions Zend/tests/lazy_objects/skipLazyInitialization.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,10 @@ getValue(): string(5) "value"
## Property [ public static $static = 'static' ]

skipInitializerForProperty():
ReflectionException: Can not use skipLazyInitialization on static property B::$static
ReflectionException: Can not use skipLazyInitialization on static property A::$static

setRawValueWithoutLazyInitialization():
ReflectionException: Can not use setRawValueWithoutLazyInitialization on static property B::$static
ReflectionException: Can not use setRawValueWithoutLazyInitialization on static property A::$static

## Property [ public $noDefault = NULL ]

Expand Down Expand Up @@ -238,10 +238,10 @@ getValue(): string(5) "value"
## Property [ public $virtual ]

skipInitializerForProperty():
ReflectionException: Can not use skipLazyInitialization on virtual property B::$virtual
ReflectionException: Can not use skipLazyInitialization on virtual property A::$virtual

setRawValueWithoutLazyInitialization():
ReflectionException: Can not use setRawValueWithoutLazyInitialization on virtual property B::$virtual
ReflectionException: Can not use setRawValueWithoutLazyInitialization on virtual property A::$virtual

## Property [ $dynamicProp ]

Expand Down Expand Up @@ -295,10 +295,10 @@ getValue(): string(5) "value"
## Property [ public static $static = 'static' ]

skipInitializerForProperty():
ReflectionException: Can not use skipLazyInitialization on static property B::$static
ReflectionException: Can not use skipLazyInitialization on static property A::$static

setRawValueWithoutLazyInitialization():
ReflectionException: Can not use setRawValueWithoutLazyInitialization on static property B::$static
ReflectionException: Can not use setRawValueWithoutLazyInitialization on static property A::$static

## Property [ public $noDefault = NULL ]

Expand Down Expand Up @@ -335,10 +335,10 @@ getValue(): string(5) "value"
## Property [ public $virtual ]

skipInitializerForProperty():
ReflectionException: Can not use skipLazyInitialization on virtual property B::$virtual
ReflectionException: Can not use skipLazyInitialization on virtual property A::$virtual

setRawValueWithoutLazyInitialization():
ReflectionException: Can not use setRawValueWithoutLazyInitialization on virtual property B::$virtual
ReflectionException: Can not use setRawValueWithoutLazyInitialization on virtual property A::$virtual

## Property [ $dynamicProp ]

Expand Down
95 changes: 61 additions & 34 deletions ext/reflection/php_reflection.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6096,6 +6096,19 @@ ZEND_METHOD(ReflectionProperty, setValue)
}
/* }}} */

/* Return the property info being used when accessing 'ref->prop' from scope
* 'scope' on 'object'. The result may be different from 'ref->prop' when the
* property is overridden on 'object' and was not private in 'scope'.
* The effective prop may add hooks or change flags. */
static zend_property_info *reflection_property_get_effective_prop(
property_reference *ref, zend_class_entry *scope, zend_object *object) {
zend_property_info *prop = ref->prop;
if (scope != object->ce && !(prop && (prop->flags & ZEND_ACC_PRIVATE))) {
prop = zend_hash_find_ptr(&object->ce->properties_info, ref->unmangled_name);
}
return prop;
}

ZEND_METHOD(ReflectionProperty, getRawValue)
{
reflection_object *intern;
Expand All @@ -6108,17 +6121,20 @@ ZEND_METHOD(ReflectionProperty, getRawValue)

GET_REFLECTION_OBJECT_PTR(ref);

if (prop_get_flags(ref) & ZEND_ACC_STATIC) {
iluuu1994 marked this conversation as resolved.
Show resolved Hide resolved
_DO_THROW("May not use getRawValue on static properties");
if (!instanceof_function(Z_OBJCE_P(object), intern->ce)) {
_DO_THROW("Given object is not an instance of the class this property was declared in");
RETURN_THROWS();
}

if (!instanceof_function(Z_OBJCE_P(object), intern->ce)) {
_DO_THROW("Given object is not an instance of the class this property was declared in");
zend_property_info *prop = reflection_property_get_effective_prop(ref,
intern->ce, Z_OBJ_P(object));

if (UNEXPECTED(prop && (prop->flags & ZEND_ACC_STATIC))) {
_DO_THROW("May not use getRawValue on static properties");
RETURN_THROWS();
}

if (!ref->prop || !ref->prop->hooks || !ref->prop->hooks[ZEND_PROPERTY_HOOK_GET]) {
if (!prop || !prop->hooks || !prop->hooks[ZEND_PROPERTY_HOOK_GET]) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason I originally used zend_read_property_ex() was to avoid replicating its behavior for things like typed uninitialized properties, and readonly properties for write. I suppose that if this gets too tedious, it might be easier to handle these two things, and just access the backing value explicitly. Luckily, prop_info->offset stays the same in child properties, and child properties cannot become virtual, so we could avoid loading the latest property info.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried that initially in #17698, but there are other cases to handle, e.g. type checking for writes, and existing code is hard to re-use (e.g. zend_assign_to_typed_prop() can not be called outside of execute_ex due to execute_data being a register and I fear that trying to make it re-usable will lead to performance regressions).

reflection_property_get_effective_prop() turns out to be quite simple, so I think I will leave it like this.

#17698 will revert potential performance regressions in the simple case.

zval rv;
zval *member_p = zend_read_property_ex(intern->ce, Z_OBJ_P(object), ref->unmangled_name, 0, &rv);

Expand All @@ -6131,17 +6147,19 @@ ZEND_METHOD(ReflectionProperty, getRawValue)
RETURN_COPY_VALUE(member_p);
}
} else {
zend_function *func = zend_get_property_hook_trampoline(ref->prop, ZEND_PROPERTY_HOOK_GET, ref->unmangled_name);
zend_function *func = zend_get_property_hook_trampoline(prop, ZEND_PROPERTY_HOOK_GET, ref->unmangled_name);
zend_call_known_instance_method_with_0_params(func, Z_OBJ_P(object), return_value);
}
}

static void reflection_property_set_raw_value(property_reference *ref, reflection_object *intern, zend_object *object, zval *value)
static void reflection_property_set_raw_value(zend_property_info *prop,
zend_string *unmangled_name, reflection_object *intern,
zend_object *object, zval *value)
{
if (!ref->prop || !ref->prop->hooks || !ref->prop->hooks[ZEND_PROPERTY_HOOK_SET]) {
zend_update_property_ex(intern->ce, object, ref->unmangled_name, value);
if (!prop || !prop->hooks || !prop->hooks[ZEND_PROPERTY_HOOK_SET]) {
zend_update_property_ex(intern->ce, object, unmangled_name, value);
} else {
zend_function *func = zend_get_property_hook_trampoline(ref->prop, ZEND_PROPERTY_HOOK_SET, ref->unmangled_name);
zend_function *func = zend_get_property_hook_trampoline(prop, ZEND_PROPERTY_HOOK_SET, unmangled_name);
zend_call_known_instance_method_with_1_params(func, object, NULL, value);
}
}
Expand All @@ -6155,55 +6173,59 @@ ZEND_METHOD(ReflectionProperty, setRawValue)

GET_REFLECTION_OBJECT_PTR(ref);

if (prop_get_flags(ref) & ZEND_ACC_STATIC) {
_DO_THROW("May not use setRawValue on static properties");
if (zend_parse_parameters(ZEND_NUM_ARGS(), "oz", &object, &value) == FAILURE) {
RETURN_THROWS();
}

if (zend_parse_parameters(ZEND_NUM_ARGS(), "oz", &object, &value) == FAILURE) {
zend_property_info *prop = reflection_property_get_effective_prop(ref,
intern->ce, Z_OBJ_P(object));

if (UNEXPECTED(prop && (prop->flags & ZEND_ACC_STATIC))) {
_DO_THROW("May not use setRawValue on static properties");
RETURN_THROWS();
}

reflection_property_set_raw_value(ref, intern, Z_OBJ_P(object), value);
reflection_property_set_raw_value(prop, ref->unmangled_name, intern, Z_OBJ_P(object), value);
}

static zend_result reflection_property_check_lazy_compatible(reflection_object *intern,
property_reference *ref, zend_object *object, const char *method)
static zend_result reflection_property_check_lazy_compatible(
zend_property_info *prop, zend_string *unmangled_name,
reflection_object *intern, zend_object *object, const char *method)
{
if (!ref->prop) {
if (!prop) {
zend_throw_exception_ex(reflection_exception_ptr, 0,
"Can not use %s on dynamic property %s::$%s",
method, ZSTR_VAL(intern->ce->name),
ZSTR_VAL(ref->unmangled_name));
ZSTR_VAL(unmangled_name));
return FAILURE;
}

if (ref->prop->flags & ZEND_ACC_STATIC) {
if (prop->flags & ZEND_ACC_STATIC) {
zend_throw_exception_ex(reflection_exception_ptr, 0,
"Can not use %s on static property %s::$%s",
method, ZSTR_VAL(intern->ce->name),
ZSTR_VAL(ref->unmangled_name));
method, ZSTR_VAL(prop->ce->name),
ZSTR_VAL(unmangled_name));
return FAILURE;
}

if (ref->prop->flags & ZEND_ACC_VIRTUAL) {
if (prop->flags & ZEND_ACC_VIRTUAL) {
zend_throw_exception_ex(reflection_exception_ptr, 0,
"Can not use %s on virtual property %s::$%s",
method, ZSTR_VAL(intern->ce->name),
ZSTR_VAL(ref->unmangled_name));
method, ZSTR_VAL(prop->ce->name),
ZSTR_VAL(unmangled_name));
return FAILURE;
}

if (UNEXPECTED(object->handlers->write_property != zend_std_write_property)) {
if (!zend_class_can_be_lazy(object->ce)) {
zend_throw_exception_ex(reflection_exception_ptr, 0,
"Can not use %s on internal class %s",
method, ZSTR_VAL(intern->ce->name));
method, ZSTR_VAL(object->ce->name));
return FAILURE;
}
}

ZEND_ASSERT(IS_VALID_PROPERTY_OFFSET(ref->prop->offset));
ZEND_ASSERT(IS_VALID_PROPERTY_OFFSET(prop->offset));

return SUCCESS;
}
Expand All @@ -6223,23 +6245,27 @@ ZEND_METHOD(ReflectionProperty, setRawValueWithoutLazyInitialization)
Z_PARAM_ZVAL(value)
} ZEND_PARSE_PARAMETERS_END();

if (reflection_property_check_lazy_compatible(intern, ref, object,
"setRawValueWithoutLazyInitialization") == FAILURE) {
RETURN_THROWS();
}

while (zend_object_is_lazy_proxy(object)
&& zend_lazy_object_initialized(object)) {
object = zend_lazy_object_get_instance(object);
}

zval *var_ptr = OBJ_PROP(object, ref->prop->offset);
zend_property_info *prop = reflection_property_get_effective_prop(ref,
intern->ce, object);

if (reflection_property_check_lazy_compatible(prop, ref->unmangled_name,
intern, object, "setRawValueWithoutLazyInitialization") == FAILURE) {
RETURN_THROWS();
}

zval *var_ptr = OBJ_PROP(object, prop->offset);
bool prop_was_lazy = Z_PROP_FLAG_P(var_ptr) & IS_PROP_LAZY;

/* Do not trigger initialization */
Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_LAZY;

reflection_property_set_raw_value(ref, intern, object, value);
reflection_property_set_raw_value(prop, ref->unmangled_name, intern, object,
value);

/* Mark property as lazy again if an exception prevented update */
if (EG(exception) && prop_was_lazy && Z_TYPE_P(var_ptr) == IS_UNDEF
Expand Down Expand Up @@ -6271,7 +6297,8 @@ ZEND_METHOD(ReflectionProperty, skipLazyInitialization)
Z_PARAM_OBJ_OF_CLASS(object, intern->ce)
} ZEND_PARSE_PARAMETERS_END();

if (reflection_property_check_lazy_compatible(intern, ref, object,
if (reflection_property_check_lazy_compatible(ref->prop,
ref->unmangled_name, intern, object,
"skipLazyInitialization") == FAILURE) {
RETURN_THROWS();
}
Expand Down
Loading
Loading