From 8368247f1864884be7d03161abf99e589e2de919 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 3 Oct 2024 21:24:17 +0200 Subject: [PATCH 1/3] Fix various hooked object iterator issues Fixes GH-16185 --- Zend/tests/property_hooks/dump.phpt | 39 ++++++--- Zend/tests/property_hooks/foreach.phpt | 30 ++++++- Zend/tests/property_hooks/gh16185.phpt | 28 +++++++ Zend/zend_property_hooks.c | 108 +++++++++++++++++-------- 4 files changed, 155 insertions(+), 50 deletions(-) create mode 100644 Zend/tests/property_hooks/gh16185.phpt diff --git a/Zend/tests/property_hooks/dump.phpt b/Zend/tests/property_hooks/dump.phpt index f05805e8861c9..d7cd57183d63f 100644 --- a/Zend/tests/property_hooks/dump.phpt +++ b/Zend/tests/property_hooks/dump.phpt @@ -59,13 +59,21 @@ function dump($test) { var_dump((array) $test); } +echo "dump(Test):\n"; dump(new Test); + +echo "\n\ndump(Child):\n"; dump(new Child); + +echo "\n\nChild::dumpTest():\n"; (new Child)->dumpTest(); + +echo "\n\nChild::dumpChild():\n"; (new Child)->dumpChild(); ?> --EXPECTF-- +dump(Test): object(Test)#%d (4) { ["addedHooks"]=> string(10) "addedHooks" @@ -102,6 +110,9 @@ array(4) { ["%0Test%0changed"]=> string(12) "changed Test" } + + +dump(Child): object(Child)#%d (5) { ["addedHooks"]=> string(10) "addedHooks" @@ -124,11 +135,11 @@ array(3) { } \Child::__set_state(array( 'addedHooks' => 'ADDEDHOOKS', - 'changed' => 'CHANGED CHILD', 'virtual' => 'VIRTUAL', 'backed' => 'BACKED', 'private' => 'PRIVATE', - 'changed' => 'changed Child', + 'changed' => 'CHANGED TEST', + 'changed' => 'CHANGED CHILD', )) {"addedHooks":"ADDEDHOOKS","virtual":"VIRTUAL","backed":"BACKED"} array(5) { @@ -143,6 +154,9 @@ array(5) { ["%0Child%0changed"]=> string(13) "changed Child" } + + +Child::dumpTest(): object(Child)#%d (5) { ["addedHooks"]=> string(10) "addedHooks" @@ -155,7 +169,7 @@ object(Child)#%d (5) { ["changed":"Child":private]=> string(13) "changed Child" } -array(4) { +array(5) { ["addedHooks"]=> string(10) "ADDEDHOOKS" ["virtual"]=> @@ -164,6 +178,8 @@ array(4) { string(6) "BACKED" ["private"]=> string(7) "PRIVATE" + ["changed"]=> + string(12) "CHANGED TEST" } array(5) { ["addedHooks"]=> @@ -179,11 +195,11 @@ array(5) { } \Child::__set_state(array( 'addedHooks' => 'ADDEDHOOKS', - 'changed' => 'CHANGED CHILD', 'virtual' => 'VIRTUAL', 'backed' => 'BACKED', 'private' => 'PRIVATE', - 'changed' => 'changed Child', + 'changed' => 'CHANGED TEST', + 'changed' => 'CHANGED CHILD', )) {"addedHooks":"ADDEDHOOKS","virtual":"VIRTUAL","backed":"BACKED"} array(5) { @@ -198,6 +214,9 @@ array(5) { ["%0Child%0changed"]=> string(13) "changed Child" } + + +Child::dumpChild(): object(Child)#%d (5) { ["addedHooks"]=> string(10) "addedHooks" @@ -210,25 +229,23 @@ object(Child)#%d (5) { ["changed":"Child":private]=> string(13) "changed Child" } -array(5) { +array(4) { ["addedHooks"]=> string(10) "ADDEDHOOKS" - ["changed"]=> - string(13) "CHANGED CHILD" ["virtual"]=> string(7) "VIRTUAL" ["backed"]=> string(6) "BACKED" ["changed"]=> - string(13) "changed Child" + string(13) "CHANGED CHILD" } \Child::__set_state(array( 'addedHooks' => 'ADDEDHOOKS', - 'changed' => 'CHANGED CHILD', 'virtual' => 'VIRTUAL', 'backed' => 'BACKED', 'private' => 'PRIVATE', - 'changed' => 'changed Child', + 'changed' => 'CHANGED TEST', + 'changed' => 'CHANGED CHILD', )) {"addedHooks":"ADDEDHOOKS","virtual":"VIRTUAL","backed":"BACKED"} array(5) { diff --git a/Zend/tests/property_hooks/foreach.phpt b/Zend/tests/property_hooks/foreach.phpt index d4b4378a32384..060a975680f88 100644 --- a/Zend/tests/property_hooks/foreach.phpt +++ b/Zend/tests/property_hooks/foreach.phpt @@ -86,8 +86,30 @@ testByVal(new ByVal); testByVal(new ByRef); testByRef(new ByRef); +class A { + private $changed { get => 'A'; } + protected $promoted { get => 'A'; } + + public function test() { + foreach ($this as $k => $v) { + var_dump($k, $v); + } + } +} + +class B extends A { + public $changed { get => 'B'; } + public $promoted { get => 'B'; } +} + +(new B)->test(); + ?> --EXPECTF-- +plain => plain +ByRef::$virtualByRef::get +virtualByRef => virtualByRef +ByRef::$virtualByRef::set ByVal::$virtualByVal::get virtualByVal => virtualByVal ByVal::$virtualByVal::set @@ -97,10 +119,6 @@ ByVal::$backed::set ByVal::$backedUninitialized::get backedUninitialized => backedUninitialized ByVal::$backedUninitialized::set -plain => plain -ByRef::$virtualByRef::get -virtualByRef => virtualByRef -ByRef::$virtualByRef::set dynamic => dynamic object(ByVal)#%d (6) { ["plain"]=> @@ -141,3 +159,7 @@ object(ByRef)#%d (3) { ["dynamic"]=> string(7) "DYNAMIC" } +string(7) "changed" +string(1) "A" +string(8) "promoted" +string(1) "B" diff --git a/Zend/tests/property_hooks/gh16185.phpt b/Zend/tests/property_hooks/gh16185.phpt new file mode 100644 index 0000000000000..d66cc095d323f --- /dev/null +++ b/Zend/tests/property_hooks/gh16185.phpt @@ -0,0 +1,28 @@ +--TEST-- +GH-16185: Incorrect indexing into dynamic property array +--FILE-- + null; + set { $this->dynamicProp = $value; } + } +} + +$object = new ByVal; +foreach ($object as $value) { + var_dump($value); + $object->_virtualByRef = $value; +} + +?> +--EXPECTF-- +NULL + +Deprecated: Creation of dynamic property ByVal::$dynamicProp is deprecated in %s on line %d +NULL diff --git a/Zend/zend_property_hooks.c b/Zend/zend_property_hooks.c index 398bf2e559e7b..21e8aa2efc79c 100644 --- a/Zend/zend_property_hooks.c +++ b/Zend/zend_property_hooks.c @@ -36,42 +36,68 @@ typedef struct { static zend_result zho_it_valid(zend_object_iterator *iter); static void zho_it_move_forward(zend_object_iterator *iter); -// FIXME: This should probably be stored on zend_class_entry somewhere (e.g. through num_virtual_props). static uint32_t zho_num_backed_props(zend_object *zobj) { - zend_property_info *prop_info; - int backed_property_count = 0; - ZEND_HASH_MAP_FOREACH_PTR(&zobj->ce->properties_info, prop_info) { - if (!(prop_info->flags & (ZEND_ACC_STATIC|ZEND_ACC_VIRTUAL))) { - backed_property_count++; - } - } ZEND_HASH_FOREACH_END(); - return backed_property_count; + return zobj->ce->default_properties_count; } -static zend_array *zho_build_properties_ex(zend_object *zobj, bool check_access, bool include_dynamic_props) +static zend_array *zho_build_properties_ex(zend_object *zobj, bool check_access, bool force_ptr, bool include_dynamic_props) { zend_class_entry *ce = zobj->ce; zend_array *properties = zend_new_array(ce->default_properties_count); zend_hash_real_init_mixed(properties); - zend_property_info *prop_info; - ZEND_HASH_MAP_FOREACH_PTR(&ce->properties_info, prop_info) { - if (prop_info->flags & ZEND_ACC_STATIC) { - continue; - } - if (check_access && zend_check_property_access(zobj, prop_info->name, false) == FAILURE) { - continue; - } - if (prop_info->hooks) { - _zend_hash_append_ptr(properties, prop_info->name, prop_info); - } else { - if (UNEXPECTED(Z_TYPE_P(OBJ_PROP(zobj, prop_info->offset)) == IS_UNDEF)) { - HT_FLAGS(properties) |= HASH_FLAG_HAS_EMPTY_IND; + /* Build list of parents */ + int32_t parent_count = 0; + for (zend_class_entry *pce = ce; pce; pce = pce->parent) { + parent_count++; + } + zend_class_entry **parents = emalloc(sizeof(zend_class_entry*) * parent_count); + int32_t i = 0; + for (zend_class_entry *pce = ce; pce; pce = pce->parent) { + parents[i++] = pce; + } + + /* Iterate parents top to bottom */ + i--; + for (; i >= 0; i--) { + zend_class_entry *pce = parents[i]; + + zend_property_info *prop_info; + ZEND_HASH_MAP_FOREACH_PTR(&pce->properties_info, prop_info) { + if (prop_info->flags & ZEND_ACC_STATIC) { + continue; } - _zend_hash_append_ind(properties, prop_info->name, OBJ_PROP(zobj, prop_info->offset)); - } - } ZEND_HASH_FOREACH_END(); + zend_string *property_name = prop_info->name; + /* When promoting properties from protected to public, use the unmangled name to preserve order. */ + if (prop_info->flags & ZEND_ACC_PROTECTED) { + const char *tmp = zend_get_unmangled_property_name(property_name); + zend_string *unmangled_name = zend_string_init(tmp, strlen(tmp), false); + if (zend_hash_exists(&ce->properties_info, unmangled_name)) { + property_name = unmangled_name; + } else { + zend_string_release(unmangled_name); + } + } + if (check_access && zend_check_property_access(zobj, property_name, false) == FAILURE) { + goto skip_property; + } + if (prop_info->hooks || force_ptr) { + zend_hash_update_ptr(properties, property_name, prop_info); + } else { + if (UNEXPECTED(Z_TYPE_P(OBJ_PROP(zobj, prop_info->offset)) == IS_UNDEF)) { + HT_FLAGS(properties) |= HASH_FLAG_HAS_EMPTY_IND; + } + zend_hash_update_ind(properties, property_name, OBJ_PROP(zobj, prop_info->offset)); + } +skip_property: + if (property_name != prop_info->name) { + zend_string_release(property_name); + } + } ZEND_HASH_FOREACH_END(); + } + + efree(parents); if (include_dynamic_props && zobj->properties) { zend_string *prop_name; @@ -93,7 +119,7 @@ ZEND_API zend_array *zend_hooked_object_build_properties(zend_object *zobj) } } - return zho_build_properties_ex(zobj, false, true); + return zho_build_properties_ex(zobj, false, false, true); } static void zho_dynamic_it_init(zend_hooked_object_iterator *hooked_iter) @@ -112,9 +138,8 @@ static void zho_declared_it_fetch_current(zend_object_iterator *iter) zend_object *zobj = Z_OBJ_P(&iter->data); zend_array *properties = Z_ARR(hooked_iter->declared_props); - zval *property = zend_hash_get_current_data(properties); - if (Z_TYPE_P(property) == IS_PTR) { - zend_property_info *prop_info = Z_PTR_P(property); + zend_property_info *prop_info = Z_PTR_P(zend_hash_get_current_data(properties)); + if (prop_info->hooks) { zend_function *get = prop_info->hooks[ZEND_PROPERTY_HOOK_GET]; if (!get && (prop_info->flags & ZEND_ACC_VIRTUAL)) { return; @@ -126,13 +151,22 @@ static void zho_declared_it_fetch_current(zend_object_iterator *iter) ZSTR_VAL(zobj->ce->name), zend_get_unmangled_property_name(prop_info->name)); return; } - zval *value = zend_read_property_ex(prop_info->ce, zobj, prop_info->name, /* silent */ true, &hooked_iter->current_data); + zend_string *unmangled_name = prop_info->name; + if (ZSTR_VAL(unmangled_name)[0] == '\0') { + const char *tmp = zend_get_unmangled_property_name(unmangled_name); + unmangled_name = zend_string_init(tmp, strlen(tmp), false); + } + zval *value = zend_read_property_ex(prop_info->ce, zobj, unmangled_name, /* silent */ true, &hooked_iter->current_data); + if (unmangled_name != prop_info->name) { + zend_string_release(unmangled_name); + } if (value == &EG(uninitialized_zval)) { return; } else if (value != &hooked_iter->current_data) { ZVAL_COPY(&hooked_iter->current_data, value); } } else { + zval *property = OBJ_PROP(zobj, prop_info->offset); ZVAL_DEINDIRECT(property); if (Z_TYPE_P(property) == IS_UNDEF) { return; @@ -141,15 +175,19 @@ static void zho_declared_it_fetch_current(zend_object_iterator *iter) ZVAL_DEREF(property); } else if (Z_TYPE_P(property) != IS_REFERENCE) { ZVAL_MAKE_REF(property); - - zend_property_info *prop_info = zend_get_property_info_for_slot(zobj, property); if (ZEND_TYPE_IS_SET(prop_info->type)) { ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(property), prop_info); } } ZVAL_COPY(&hooked_iter->current_data, property); } - zend_hash_get_current_key_zval(properties, &hooked_iter->current_key); + + if (ZSTR_VAL(prop_info->name)[0] == '\0') { + const char *tmp = zend_get_unmangled_property_name(prop_info->name); + ZVAL_STR(&hooked_iter->current_key, zend_string_init(tmp, strlen(tmp), false)); + } else { + ZVAL_STR_COPY(&hooked_iter->current_key, prop_info->name); + } } static void zho_dynamic_it_fetch_current(zend_object_iterator *iter) @@ -312,7 +350,7 @@ ZEND_API zend_object_iterator *zend_hooked_object_get_iterator(zend_class_entry iterator->it.funcs = &zend_hooked_object_it_funcs; iterator->by_ref = by_ref; iterator->declared_props_done = false; - zend_array *properties = zho_build_properties_ex(zobj, true, false); + zend_array *properties = zho_build_properties_ex(zobj, true, true, false); ZVAL_ARR(&iterator->declared_props, properties); zho_dynamic_it_init(iterator); ZVAL_UNDEF(&iterator->current_key); From af9e55be14cbd28bac57690d38e2787ad0646db6 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 8 Oct 2024 11:05:05 +0200 Subject: [PATCH 2/3] Fix visibility of unpromoted protected property --- Zend/tests/property_hooks/foreach.phpt | 3 +++ Zend/zend_property_hooks.c | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/Zend/tests/property_hooks/foreach.phpt b/Zend/tests/property_hooks/foreach.phpt index 060a975680f88..92a5bcb96e050 100644 --- a/Zend/tests/property_hooks/foreach.phpt +++ b/Zend/tests/property_hooks/foreach.phpt @@ -89,6 +89,7 @@ testByRef(new ByRef); class A { private $changed { get => 'A'; } protected $promoted { get => 'A'; } + protected $protected { get => 'A'; } public function test() { foreach ($this as $k => $v) { @@ -163,3 +164,5 @@ string(7) "changed" string(1) "A" string(8) "promoted" string(1) "B" +string(9) "protected" +string(1) "A" diff --git a/Zend/zend_property_hooks.c b/Zend/zend_property_hooks.c index 21e8aa2efc79c..7aa0e85d29020 100644 --- a/Zend/zend_property_hooks.c +++ b/Zend/zend_property_hooks.c @@ -73,7 +73,8 @@ static zend_array *zho_build_properties_ex(zend_object *zobj, bool check_access, if (prop_info->flags & ZEND_ACC_PROTECTED) { const char *tmp = zend_get_unmangled_property_name(property_name); zend_string *unmangled_name = zend_string_init(tmp, strlen(tmp), false); - if (zend_hash_exists(&ce->properties_info, unmangled_name)) { + zend_property_info *child_prop_info = zend_hash_find_ptr(&ce->properties_info, unmangled_name); + if (child_prop_info && (child_prop_info->flags & ZEND_ACC_PUBLIC)) { property_name = unmangled_name; } else { zend_string_release(unmangled_name); From c1fc935fbb360e10eed76b8e2759187f0ddc0153 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 8 Oct 2024 12:06:44 +0200 Subject: [PATCH 3/3] Fix shadowing and reference to readonly --- Zend/tests/property_hooks/foreach.phpt | 8 +++++- Zend/tests/property_hooks/gh16185_002.phpt | 30 ++++++++++++++++++++++ Zend/zend_property_hooks.c | 15 +++++++++-- 3 files changed, 50 insertions(+), 3 deletions(-) create mode 100644 Zend/tests/property_hooks/gh16185_002.phpt diff --git a/Zend/tests/property_hooks/foreach.phpt b/Zend/tests/property_hooks/foreach.phpt index 92a5bcb96e050..e40f2c1d4d57e 100644 --- a/Zend/tests/property_hooks/foreach.phpt +++ b/Zend/tests/property_hooks/foreach.phpt @@ -90,6 +90,7 @@ class A { private $changed { get => 'A'; } protected $promoted { get => 'A'; } protected $protected { get => 'A'; } + private $shadowed = 'A'; public function test() { foreach ($this as $k => $v) { @@ -98,12 +99,15 @@ class A { } } +#[AllowDynamicProperties] class B extends A { public $changed { get => 'B'; } public $promoted { get => 'B'; } } -(new B)->test(); +$b = new B; +$b->shadowed = 'Global'; +$b->test(); ?> --EXPECTF-- @@ -166,3 +170,5 @@ string(8) "promoted" string(1) "B" string(9) "protected" string(1) "A" +string(8) "shadowed" +string(1) "A" diff --git a/Zend/tests/property_hooks/gh16185_002.phpt b/Zend/tests/property_hooks/gh16185_002.phpt new file mode 100644 index 0000000000000..39edba438b04c --- /dev/null +++ b/Zend/tests/property_hooks/gh16185_002.phpt @@ -0,0 +1,30 @@ +--TEST-- +GH-16185: Hooked object iterator with readonly props +--FILE-- +prop = 1; + } +} + +$c = new C; + +// Okay, as foreach skips over uninitialized properties. +foreach ($c as &$prop) {} + +$c->init(); + +try { + foreach ($c as &$prop) {} +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +?> +--EXPECTF-- +Cannot acquire reference to readonly property C::$prop diff --git a/Zend/zend_property_hooks.c b/Zend/zend_property_hooks.c index 7aa0e85d29020..0286bc0c4486f 100644 --- a/Zend/zend_property_hooks.c +++ b/Zend/zend_property_hooks.c @@ -175,6 +175,12 @@ static void zho_declared_it_fetch_current(zend_object_iterator *iter) if (!hooked_iter->by_ref) { ZVAL_DEREF(property); } else if (Z_TYPE_P(property) != IS_REFERENCE) { + if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { + zend_throw_error(NULL, + "Cannot acquire reference to readonly property %s::$%s", + ZSTR_VAL(prop_info->ce->name), zend_get_unmangled_property_name(prop_info->name)); + return; + } ZVAL_MAKE_REF(property); if (ZEND_TYPE_IS_SET(prop_info->type)) { ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(property), prop_info); @@ -208,6 +214,11 @@ static void zho_dynamic_it_fetch_current(zend_object_iterator *iter) return; } + zend_object *zobj = Z_OBJ_P(&hooked_iter->it.data); + if (bucket->key && zend_check_property_access(zobj, bucket->key, true) != SUCCESS) { + return; + } + if (hooked_iter->by_ref && Z_TYPE(bucket->val) != IS_REFERENCE) { ZVAL_MAKE_REF(&bucket->val); } @@ -305,9 +316,9 @@ static void zho_it_rewind(zend_object_iterator *iter) zval_ptr_dtor_nogc(&hooked_iter->current_key); ZVAL_UNDEF(&hooked_iter->current_key); - hooked_iter->declared_props_done = false; zend_array *properties = Z_ARR(hooked_iter->declared_props); zend_hash_internal_pointer_reset(properties); + hooked_iter->declared_props_done = !zend_hash_num_elements(properties); hooked_iter->dynamic_props_done = false; EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = zho_num_backed_props(Z_OBJ(iter->data)); } @@ -350,9 +361,9 @@ ZEND_API zend_object_iterator *zend_hooked_object_get_iterator(zend_class_entry ZVAL_OBJ_COPY(&iterator->it.data, zobj); iterator->it.funcs = &zend_hooked_object_it_funcs; iterator->by_ref = by_ref; - iterator->declared_props_done = false; zend_array *properties = zho_build_properties_ex(zobj, true, true, false); ZVAL_ARR(&iterator->declared_props, properties); + iterator->declared_props_done = !zend_hash_num_elements(properties); zho_dynamic_it_init(iterator); ZVAL_UNDEF(&iterator->current_key); ZVAL_UNDEF(&iterator->current_data);