From 376e90fbf27c23757bdda6a2a43f0330c9eebff4 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 20 Feb 2025 21:50:42 +0100 Subject: [PATCH] Fix circumvented added hooks in JIT 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 GH-17376 Closes GH-17870 --- NEWS | 2 + Zend/tests/property_hooks/gh17376.phpt | 119 +++++++++++++++++++++++++ Zend/zend_compile.h | 2 +- Zend/zend_inheritance.c | 41 +++++++-- Zend/zend_lazy_objects.c | 6 +- Zend/zend_objects_API.c | 12 +++ Zend/zend_objects_API.h | 14 ++- 7 files changed, 184 insertions(+), 12 deletions(-) create mode 100644 Zend/tests/property_hooks/gh17376.phpt diff --git a/NEWS b/NEWS index caad7e6afff..285bf4e23a8 100644 --- a/NEWS +++ b/NEWS @@ -7,6 +7,8 @@ PHP NEWS (ilutov) . Fixed accidentally inherited default value in overridden virtual properties. (ilutov) + . Fixed bug GH-17376 (Broken JIT polymorphism for property hooks added to + child class). (ilutov) 27 Feb 2025, PHP 8.4.5 diff --git a/Zend/tests/property_hooks/gh17376.phpt b/Zend/tests/property_hooks/gh17376.phpt new file mode 100644 index 00000000000..d0896ff4599 --- /dev/null +++ b/Zend/tests/property_hooks/gh17376.phpt @@ -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-- +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 diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h index 1eaf3ef686e..7f425852d01 100644 --- a/Zend/zend_compile.h +++ b/Zend/zend_compile.h @@ -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 */ diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index db92426a807..b1ed8e4b616 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1478,14 +1478,32 @@ 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; + } + int parent_num = OBJ_PROP_TO_NUM(parent_info->offset); if (child_info->offset != ZEND_VIRTUAL_PROPERTY_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]); + } } else { /* Default value was removed in child, remove it from parent too. */ if (ZEND_TYPE_IS_SET(child_info->type)) { @@ -1495,7 +1513,9 @@ static void do_inherit_property(zend_property_info *parent_info, zend_string *ke } } - child_info->offset = parent_info->offset; + if (!use_child_prop) { + child_info->offset = parent_info->offset; + } child_info->flags &= ~ZEND_ACC_VIRTUAL; } @@ -1670,7 +1690,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(); } @@ -1684,8 +1705,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. */ diff --git a/Zend/zend_lazy_objects.c b/Zend/zend_lazy_objects.c index e6dd6144d4b..41ba3e1cd28 100644 --- a/Zend/zend_lazy_objects.c +++ b/Zend/zend_lazy_objects.c @@ -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)) { diff --git a/Zend/zend_objects_API.c b/Zend/zend_objects_API.c index 8a6b714c8b3..1ba250bec64 100644 --- a/Zend/zend_objects_API.c +++ b/Zend/zend_objects_API.c @@ -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; +} diff --git a/Zend/zend_objects_API.h b/Zend/zend_objects_API.h index 02326f2b314..242bf212ba9 100644 --- a/Zend/zend_objects_API.h +++ b/Zend/zend_objects_API.h @@ -96,6 +96,8 @@ 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) @@ -103,7 +105,11 @@ static inline zend_property_info *zend_get_property_info_for_slot_self(zend_obje 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) @@ -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. */