Skip to content

Fix circumvented added hooks in JIT #17870

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 5 commits into from
Closed

Fix circumvented added hooks in JIT #17870

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
add3e2b
Fix circumvented added hooks in JIT
iluuu1994 Feb 20, 2025
7042ec6
Fix prop_info_offset calculation
iluuu1994 Feb 21, 2025
a48f879
[skip ci] Adjust comment
iluuu1994 Feb 22, 2025
060a409
Revert unnecessary changes
iluuu1994 Feb 24, 2025
8ca7bc4
Fix multi-level inheritance with hooks
iluuu1994 Feb 25, 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
2 changes: 1 addition & 1 deletion Zend/tests/property_hooks/dump.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ class Test {
}

class Child extends Test {
public $addedHooks {
public $addedHooks = 'addedHooks' {
get { return strtoupper(parent::$addedHooks::get()); }
}
private $changed = 'changed Child' {
Expand Down
8 changes: 5 additions & 3 deletions Zend/tests/property_hooks/generator_hook_002.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ class A {
class B extends A {
public $prop {
get {
yield parent::$prop::get();
yield parent::$prop::get() + 1;
yield parent::$prop::get() + 2;
yield parent::$prop::get() + 3;
Expand All @@ -24,6 +25,7 @@ foreach ($b->prop as $value) {

?>
--EXPECT--
int(43)
int(44)
int(45)
NULL
int(1)
int(2)
int(3)
119 changes: 119 additions & 0 deletions Zend/tests/property_hooks/gh17376.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,119 @@
--TEST--
GH-17376: Child classes may add hooks to plain properties
--INI--
# Avoid triggering for main, trigger for test so we get a side-trace for property hooks
opcache.jit_hot_func=2
--FILE--
<?php

class A {
public $prop = 1;
}

class B extends A {
public $prop = 1 {
get {
echo __METHOD__, "\n";
return $this->prop;
}
set {
echo __METHOD__, "\n";
$this->prop = $value;
}
}
}

function test(A $a) {
echo "read\n";
var_dump($a->prop);
echo "write\n";
$a->prop = 42;
echo "read-write\n";
$a->prop += 43;
echo "pre-inc\n";
++$a->prop;
echo "pre-dec\n";
--$a->prop;
echo "post-inc\n";
$a->prop++;
echo "post-dec\n";
$a->prop--;
}

echo "A\n";
test(new A);

echo "\nA\n";
test(new A);

echo "\nB\n";
test(new B);

echo "\nB\n";
test(new B);

?>
--EXPECT--
A
read
int(1)
write
read-write
pre-inc
pre-dec
post-inc
post-dec

A
read
int(1)
write
read-write
pre-inc
pre-dec
post-inc
post-dec

B
read
B::$prop::get
int(1)
write
B::$prop::set
read-write
B::$prop::get
B::$prop::set
pre-inc
B::$prop::get
B::$prop::set
pre-dec
B::$prop::get
B::$prop::set
post-inc
B::$prop::get
B::$prop::set
post-dec
B::$prop::get
B::$prop::set

B
read
B::$prop::get
int(1)
write
B::$prop::set
read-write
B::$prop::get
B::$prop::set
pre-inc
B::$prop::get
B::$prop::set
pre-dec
B::$prop::get
B::$prop::set
post-inc
B::$prop::get
B::$prop::set
post-dec
B::$prop::get
B::$prop::set
29 changes: 29 additions & 0 deletions Zend/tests/property_hooks/gh17376_2.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
--TEST--
GH-17376: Mutli-level inheritance with property hooks
--FILE--
<?php

class A {
public $prop = 1;
}

class B extends A {
public $prop = 1 { get => parent::$prop::get() * 2; }
}

class C extends B {
public $prop = 3;
}

function test(A $a) {
var_dump((array)$a);
}

test(new C);

?>
--EXPECT--
array(1) {
["prop"]=>
int(3)
}
2 changes: 1 addition & 1 deletion Zend/tests/property_hooks/parent_get_plain.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class P {
}

class C extends P {
public $prop {
public $prop = 42 {
Copy link
Member

Choose a reason for hiding this comment

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

Why do you need these tests changes?
Does the patch break some previous behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this patch also fixes a bug. Properties do not inherit default values, but we accidentally did for properties with hooks.

Copy link
Member

Choose a reason for hiding this comment

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

If the fix may be done separately, it's better to do this separately (add bug report and so on).
If the fix relays on new inheritance logic, and can't be simple separated, don't do this.

get => parent::$prop::get();
}
}
Expand Down
2 changes: 1 addition & 1 deletion Zend/zend_compile.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ typedef struct _zend_property_info {
#define OBJ_PROP_TO_OFFSET(num) \
((uint32_t)(XtOffsetOf(zend_object, properties_table) + sizeof(zval) * (num)))
#define OBJ_PROP_TO_NUM(offset) \
((offset - OBJ_PROP_TO_OFFSET(0)) / sizeof(zval))
(((offset) - OBJ_PROP_TO_OFFSET(0)) / sizeof(zval))

typedef struct _zend_class_constant {
zval value; /* flags are stored in u2 */
Expand Down
42 changes: 34 additions & 8 deletions Zend/zend_inheritance.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1436,7 +1436,7 @@ static void do_inherit_property(zend_property_info *parent_info, zend_string *ke
}
if (!(parent_info->flags & ZEND_ACC_PRIVATE)) {
if (!(parent_info->ce->ce_flags & ZEND_ACC_INTERFACE)) {
child_info->prototype = parent_info;
child_info->prototype = parent_info->prototype;
}

if (UNEXPECTED((parent_info->flags & ZEND_ACC_STATIC) != (child_info->flags & ZEND_ACC_STATIC))) {
Expand Down Expand Up @@ -1478,17 +1478,38 @@ static void do_inherit_property(zend_property_info *parent_info, zend_string *ke
zend_error_noreturn(E_COMPILE_ERROR, "Access level to %s::$%s must be %s (as in class %s)%s", ZSTR_VAL(ce->name), ZSTR_VAL(key), zend_visibility_string(parent_info->flags), ZSTR_VAL(parent_info->ce->name), (parent_info->flags&ZEND_ACC_PUBLIC) ? "" : " or weaker");
}
if (!(child_info->flags & ZEND_ACC_STATIC) && !(parent_info->flags & ZEND_ACC_VIRTUAL)) {
/* If we added hooks to the child property, we use the child's slot for
* storage to keep the parent slot set to IS_UNDEF. This automatically
* picks the slow path in the JIT. */
bool use_child_prop = !parent_info->hooks && child_info->hooks;

if (use_child_prop && child_info->offset == ZEND_VIRTUAL_PROPERTY_OFFSET) {
child_info->offset = OBJ_PROP_TO_OFFSET(ce->default_properties_count);
ce->default_properties_count++;
ce->default_properties_table = perealloc(ce->default_properties_table, sizeof(zval) * ce->default_properties_count, ce->type == ZEND_INTERNAL_CLASS);
Copy link
Member

Choose a reason for hiding this comment

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

Doing reallocs with linear size increases is not very efficient, but okay this should be rare and happens during inheritance so it shouldn't be a big problem / showstopper.

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 fully agree ofc. That said, this excerpt comes from zend_declare_typed_property() which will trigger much more commonly than this code path.

zval *property_default_ptr = &ce->default_properties_table[OBJ_PROP_TO_NUM(child_info->offset)];
ZVAL_UNDEF(property_default_ptr);
Z_PROP_FLAG_P(property_default_ptr) = IS_PROP_UNINIT;
}

if (child_info->offset != ZEND_VIRTUAL_PROPERTY_OFFSET) {
int parent_num = OBJ_PROP_TO_NUM(parent_info->offset);
int child_num = OBJ_PROP_TO_NUM(child_info->offset);

/* Don't keep default properties in GC (they may be freed by opcache) */
zval_ptr_dtor_nogc(&(ce->default_properties_table[parent_num]));
ce->default_properties_table[parent_num] = ce->default_properties_table[child_num];
ZVAL_UNDEF(&ce->default_properties_table[child_num]);

if (use_child_prop) {
ZVAL_UNDEF(&ce->default_properties_table[parent_num]);
} else {
int child_num = OBJ_PROP_TO_NUM(child_info->offset);
ce->default_properties_table[parent_num] = ce->default_properties_table[child_num];
ZVAL_UNDEF(&ce->default_properties_table[child_num]);
}
}

child_info->offset = parent_info->offset;
if (!use_child_prop) {
child_info->offset = parent_info->offset;
}
child_info->flags &= ~ZEND_ACC_VIRTUAL;
}

Expand Down Expand Up @@ -1663,7 +1684,8 @@ void zend_build_properties_info_table(zend_class_entry *ce)
ZEND_HASH_MAP_FOREACH_PTR(&ce->properties_info, prop) {
if (prop->ce == ce && (prop->flags & ZEND_ACC_STATIC) == 0
&& !(prop->flags & ZEND_ACC_VIRTUAL)) {
table[OBJ_PROP_TO_NUM(prop->offset)] = prop;
uint32_t prop_table_offset = OBJ_PROP_TO_NUM(!(prop->prototype->flags & ZEND_ACC_VIRTUAL) ? prop->prototype->offset : prop->offset);
table[prop_table_offset] = prop;
}
} ZEND_HASH_FOREACH_END();
}
Expand All @@ -1677,8 +1699,12 @@ ZEND_API void zend_verify_hooked_property(zend_class_entry *ce, zend_property_in
/* We specified a default value (otherwise offset would be -1), but the virtual flag wasn't
* removed during inheritance. */
if ((prop_info->flags & ZEND_ACC_VIRTUAL) && prop_info->offset != ZEND_VIRTUAL_PROPERTY_OFFSET) {
zend_error_noreturn(E_COMPILE_ERROR,
"Cannot specify default value for virtual hooked property %s::$%s", ZSTR_VAL(ce->name), ZSTR_VAL(prop_name));
if (Z_TYPE(ce->default_properties_table[OBJ_PROP_TO_NUM(prop_info->offset)]) == IS_UNDEF) {
prop_info->offset = ZEND_VIRTUAL_PROPERTY_OFFSET;
} else {
zend_error_noreturn(E_COMPILE_ERROR,
"Cannot specify default value for virtual hooked property %s::$%s", ZSTR_VAL(ce->name), ZSTR_VAL(prop_name));
}
}
/* If the property turns backed during inheritance and no type and default value are set, we want
* the default value to be null. */
Expand Down
6 changes: 5 additions & 1 deletion Zend/zend_lazy_objects.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -806,7 +806,11 @@ zend_property_info *zend_lazy_object_get_property_info_for_slot(zend_object *obj
zend_property_info **table = obj->ce->properties_info_table;
intptr_t prop_num = slot - obj->properties_table;
if (prop_num >= 0 && prop_num < obj->ce->default_properties_count) {
return table[prop_num];
if (table[prop_num]) {
Copy link
Member

Choose a reason for hiding this comment

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

When table[prop_num] may be NULL?
Only for regular properties overridden with hooked ones?

Copy link
Member Author

Choose a reason for hiding this comment

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

Only for regular properties overridden with hooked ones?

They will only ever be NULL for properties with hooks that override properties without hooks. This could add an EXPECTED() guard, if you like.

Copy link
Member

Choose a reason for hiding this comment

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

OK. I don't care.

return table[prop_num];
} else {
return zend_get_property_info_for_slot_slow(obj, slot);
}
}

if (!zend_lazy_object_initialized(obj)) {
Expand Down
12 changes: 12 additions & 0 deletions Zend/zend_objects_API.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,3 +200,15 @@ ZEND_API void ZEND_FASTCALL zend_objects_store_del(zend_object *object) /* {{{ *
}
}
/* }}} */

ZEND_API ZEND_COLD zend_property_info *zend_get_property_info_for_slot_slow(zend_object *obj, zval *slot)
{
uintptr_t offset = (uintptr_t)slot - (uintptr_t)obj->properties_table;
zend_property_info *prop_info;
ZEND_HASH_MAP_FOREACH_PTR(&obj->ce->properties_info, prop_info) {
if (prop_info->offset == offset) {
return prop_info;
}
} ZEND_HASH_FOREACH_END();
Comment on lines +208 to +212
Copy link
Member

Choose a reason for hiding this comment

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

Think, if it's possible to improve this linear search.
This is not a big problem or stopper, but it would be good o avoid this.

May be we may store the prop_info somehow differently...
Or may be check only the child properties (skipping ce->parrent->default_properties_count)...

Copy link
Member Author

Choose a reason for hiding this comment

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

May be we may store the prop_info somehow differently...

A straight-forward solution in that regard would be to store a clone of properties_info_table that sets both the parent and child offset to the child property. This was not done in properties_info_table to avoid breaking iteration over the table. That would not be ABI compatible though (modification of zend_class_entry), so should likely be done in master only.

Copy link
Member

Choose a reason for hiding this comment

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

Checking only child properties will already be a welcome improvement

return NULL;
}
14 changes: 12 additions & 2 deletions Zend/zend_objects_API.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,14 +96,20 @@ static zend_always_inline void *zend_object_alloc(size_t obj_size, zend_class_en
return obj;
}

ZEND_API ZEND_COLD zend_property_info *zend_get_property_info_for_slot_slow(zend_object *obj, zval *slot);

/* Use when 'slot' was obtained directly from obj->properties_table, or when
* 'obj' can not be lazy. Otherwise, use zend_get_property_info_for_slot(). */
static inline zend_property_info *zend_get_property_info_for_slot_self(zend_object *obj, zval *slot)
{
zend_property_info **table = obj->ce->properties_info_table;
intptr_t prop_num = slot - obj->properties_table;
ZEND_ASSERT(prop_num >= 0 && prop_num < obj->ce->default_properties_count);
return table[prop_num];
if (table[prop_num]) {
return table[prop_num];
} else {
return zend_get_property_info_for_slot_slow(obj, slot);
}
}

static inline zend_property_info *zend_get_property_info_for_slot(zend_object *obj, zval *slot)
Expand All @@ -114,7 +120,11 @@ static inline zend_property_info *zend_get_property_info_for_slot(zend_object *o
zend_property_info **table = obj->ce->properties_info_table;
intptr_t prop_num = slot - obj->properties_table;
ZEND_ASSERT(prop_num >= 0 && prop_num < obj->ce->default_properties_count);
return table[prop_num];
if (table[prop_num]) {
return table[prop_num];
} else {
return zend_get_property_info_for_slot_slow(obj, slot);
}
}

/* Helper for cases where we're only interested in property info of typed properties. */
Expand Down
Loading