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.

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 circumvented added hooks in JIT #17870

Open
wants to merge 4 commits into
base: PHP-8.4
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
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
2 changes: 1 addition & 1 deletion Zend/zend_compile.h
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
40 changes: 33 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 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);
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
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
Loading