Skip to content

Commit

Permalink
Fix circumvented added hooks in JIT
Browse files Browse the repository at this point in the history
The following code poses a problem in the JIT:

```php
class A {
    public $prop = 1;
}

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

function test(A $a) {
    var_dump($a->prop);
}

test(new B);
```

The JIT would assume A::$prop in test() could be accessed directly
through OBJ_PROP_NUM(). However, since child classes can add new hooks
to existing properties, this assumption no longer holds.

To avoid introducing more JIT checks, a hooked property that overrides a
unhooked property now results in a separate zval slot that is used
instead of the parent slot. This causes the JIT to pick the slow path
due to an IS_UNDEF value in the parent slot.

zend_class_entry.properties_info_table poses a problem in that
zend_get_property_info_for_slot() and friends will be called using the
child slot, which does not store its property info, since the parent
slot already does. In this case, zend_get_property_info_for_slot() now
provides a fallback that will iterate all property infos to find the
correct one.

This also uncovered a bug (see Zend/tests/property_hooks/dump.phpt)
where the default value of a parent property would accidentally be
inherited by the child property.

Fixes phpGH-17376
  • Loading branch information
iluuu1994 committed Feb 21, 2025
1 parent 1eb6751 commit add3e2b
Show file tree
Hide file tree
Showing 10 changed files with 195 additions and 20 deletions.
2 changes: 1 addition & 1 deletion Zend/tests/property_hooks/dump.phpt
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
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
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
2 changes: 1 addition & 1 deletion Zend/tests/property_hooks/parent_get_plain.phpt
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 {
get => parent::$prop::get();
}
}
Expand Down
5 changes: 4 additions & 1 deletion Zend/zend_compile.h
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,10 @@ 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))

#define Z_PROP_TABLE_OFFSET(prop_info) \
OBJ_PROP_TO_NUM(!((prop_info)->prototype->flags & ZEND_ACC_VIRTUAL) ? (prop_info)->prototype->offset : (prop_info)->offset)

typedef struct _zend_class_constant {
zval value; /* flags are stored in u2 */
Expand Down
39 changes: 32 additions & 7 deletions Zend/zend_inheritance.c
Original file line number Diff line number Diff line change
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 props to the child property, we use the childs slot for
* storage to keep the parent slot set to null. 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);
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,7 @@ 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;
table[Z_PROP_TABLE_OFFSET(prop)] = prop;
}
} ZEND_HASH_FOREACH_END();
}
Expand All @@ -1677,8 +1698,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
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]) {
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
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();
return NULL;
}
14 changes: 12 additions & 2 deletions Zend/zend_objects_API.h
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
8 changes: 4 additions & 4 deletions ext/opcache/jit/zend_jit_ir.c
Original file line number Diff line number Diff line change
Expand Up @@ -14377,7 +14377,7 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit,
ref = ir_CONST_ADDR(prop_info);
} else {
int prop_info_offset =
(((prop_info->offset - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));
(((Z_PROP_TABLE_OFFSET(prop_info) - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));

ref = ir_LOAD_A(ir_ADD_OFFSET(obj_ref, offsetof(zend_object, ce)));
ref = ir_LOAD_A(ir_ADD_OFFSET(ref, offsetof(zend_class_entry, properties_info_table)));
Expand Down Expand Up @@ -14778,7 +14778,7 @@ static int zend_jit_assign_obj(zend_jit_ctx *jit,
ref = ir_CONST_ADDR(prop_info);
} else {
int prop_info_offset =
(((prop_info->offset - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));
(((Z_PROP_TABLE_OFFSET(prop_info) - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));

ref = ir_LOAD_A(ir_ADD_OFFSET(obj_ref, offsetof(zend_object, ce)));
ref = ir_LOAD_A(ir_ADD_OFFSET(ref, offsetof(zend_class_entry, properties_info_table)));
Expand Down Expand Up @@ -15134,7 +15134,7 @@ static int zend_jit_assign_obj_op(zend_jit_ctx *jit,
ref = ir_CONST_ADDR(prop_info);
} else {
int prop_info_offset =
(((prop_info->offset - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));
(((Z_PROP_TABLE_OFFSET(prop_info) - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));

ref = ir_LOAD_A(ir_ADD_OFFSET(obj_ref, offsetof(zend_object, ce)));
ref = ir_LOAD_A(ir_ADD_OFFSET(ref, offsetof(zend_class_entry, properties_info_table)));
Expand Down Expand Up @@ -15524,7 +15524,7 @@ static int zend_jit_incdec_obj(zend_jit_ctx *jit,
ref = ir_CONST_ADDR(prop_info);
} else {
int prop_info_offset =
(((prop_info->offset - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));
(((Z_PROP_TABLE_OFFSET(prop_info) - (sizeof(zend_object) - sizeof(zval))) / sizeof(zval)) * sizeof(void*));

ref = ir_LOAD_A(ir_ADD_OFFSET(obj_ref, offsetof(zend_object, ce)));
ref = ir_LOAD_A(ir_ADD_OFFSET(ref, offsetof(zend_class_entry, properties_info_table)));
Expand Down

0 comments on commit add3e2b

Please sign in to comment.