Skip to content

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

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

Closed
wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 2 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
89 changes: 58 additions & 31 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) {
_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,42 +6173,46 @@ 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));
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));
ZSTR_VAL(unmangled_name));
return FAILURE;
}

Expand All @@ -6203,7 +6225,7 @@ static zend_result reflection_property_check_lazy_compatible(reflection_object *
}
}

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
169 changes: 169 additions & 0 deletions ext/reflection/tests/property_hooks/gh17713.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,169 @@
--TEST--
GH-17713: getRawValue(), setRawValue(), setRawValueWithoutLazyInitialization() may call hooks
--FILE--
<?php

#[AllowDynamicProperties]
class Base {
public $publicProp = 'Base::$publicProp';
private $privateProp = 'Base::$privateProp';
public $hookedProp {
get {
echo __METHOD__, "\n";
return $this->hookedProp;
}
set {
echo __METHOD__, "\n";
$this->hookedProp = $value;
}
}
public $virtualProp {
get {
echo __METHOD__, "\n";
return __METHOD__;
}
set {
echo __METHOD__, "\n";
}
}
}

class Test extends Base {
public $publicProp {
get {
echo __FUNCTION__, "\n";
return $this->publicProp;
}
set {
echo __FUNCTION__, "\n";
$this->publicProp = $value;
}
}
private $privateProp = 'Test::$privateProp' {
get {
echo __FUNCTION__, "\n";
return $this->privateProp;
}
set {
echo __FUNCTION__, "\n";
$this->privateProp = $value;
}
}
public $dynamicProp {
get {
echo __FUNCTION__, "\n";
return $this->dynamicProp;
}
set {
echo __FUNCTION__, "\n";
$this->dynamicProp = $value;
}
}
public $hookedProp {
get {
echo __METHOD__, "\n";
return $this->hookedProp;
}
set {
echo __METHOD__, "\n";
$this->hookedProp = $value;
}
}
public $virtualProp {
get {
echo __METHOD__, "\n";
return $this->virtualProp;
}
set {
echo __METHOD__, "\n";
$this->virtualProp = $value;
}
}
public static $changedProp = 'Test::$changedProp';
}

function test($scope, $class, $prop) {
printf(
"# Accessing %s->%s from scope %s\n",
$class,
$prop,
is_object($scope) ? get_class($scope) : $scope,
);

$propertyReflection = new ReflectionProperty($scope, $prop);
$object = new $class();
try {
$propertyReflection->setRawValue($object, 42);
} catch (Throwable $e) {
echo $e->getMessage(), "\n";
}
try {
var_dump($propertyReflection->getRawValue($object));
} catch (Throwable $e) {
echo $e->getMessage(), "\n";
}

try {
$propertyReflection->setRawValueWithoutLazyInitialization($object, 43);
} catch (Throwable $e) {
echo $e->getMessage(), "\n";
}
try {
var_dump($propertyReflection->getRawValue($object));
} catch (Throwable $e) {
echo $e->getMessage(), "\n";
}
if ($prop === 'privateProp') {
printf(
"# Accessing %s->%s again from scope %s\n",
$class,
$prop,
$class,
);
$propertyReflection = new ReflectionProperty($class, $prop);
var_dump($propertyReflection->getRawValue($object));
}
}

test(Base::class, Test::class, 'publicProp');
test(Base::class, Test::class, 'privateProp');
test(Base::class, Test::class, 'hookedProp');
test(Base::class, Test::class, 'virtualProp');
test(Base::class, Base::class, 'virtualProp');

$scope = new Base();
$scope->dynamicProp = 'Base::$dynamicProp';
$scope->changedProp = 'Base::$dynamicProp';

test($scope, Test::class, 'dynamicProp');
test($scope, Test::class, 'changedProp');

?>
--EXPECT--
# Accessing Test->publicProp from scope Base
int(42)
int(43)
# Accessing Test->privateProp from scope Base
int(42)
int(43)
# Accessing Test->privateProp again from scope Test
string(18) "Test::$privateProp"
# Accessing Test->hookedProp from scope Base
int(42)
int(43)
# Accessing Test->virtualProp from scope Base
int(42)
int(43)
# Accessing Base->virtualProp from scope Base
Must not write to virtual property Base::$virtualProp
Must not read from virtual property Base::$virtualProp
Can not use setRawValueWithoutLazyInitialization on virtual property Base::$virtualProp
Must not read from virtual property Base::$virtualProp
# Accessing Test->dynamicProp from scope Base
int(42)
int(43)
# Accessing Test->changedProp from scope Base
May not use setRawValue on static properties
May not use getRawValue on static properties
Can not use setRawValueWithoutLazyInitialization on static property Base::$changedProp
May not use getRawValue on static properties