Fix various hooked object iterator issues

Fixes GH-16185 Closes GH-16281
2026-03-24 00:02:20 +01:00 · 2024-10-03 21:24:17 +02:00
parent 3fcf8caca8
commit d76ef13757
6 changed files with 259 additions and 102 deletions
--- a/1
+++ b/1
@@ -37,6 +37,7 @@ PHP                                                                        NEWS
  . Fixed bug GH-16188 (Assertion failure in Zend/zend_exceptions.c). (Arnaud)
  . Fixed bug GH-16233 (Observer segfault when calling user function in
    internal function via trampoline). (nielsdos)
+  . Fixed bug GH-16185 (Various hooked object iterator issues). (ilutov)

 - DOM:
  . Fixed bug GH-16039 (Segmentation fault (access null pointer) in
--- 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,69 @@ array(5) {
  ["%0Child%0changed"]=>
  string(13) "changed Child"
 }
+
+
+Child::dumpTest():
+object(Child)#%d (5) {
+  ["addedHooks"]=>
+  string(10) "addedHooks"
+  ["backed"]=>
+  string(6) "backed"
+  ["private":"Test":private]=>
+  string(7) "private"
+  ["changed":"Test":private]=>
+  string(12) "changed Test"
+  ["changed":"Child":private]=>
+  string(13) "changed Child"
+}
+array(5) {
+  ["addedHooks"]=>
+  string(10) "ADDEDHOOKS"
+  ["virtual"]=>
+  string(7) "VIRTUAL"
+  ["backed"]=>
+  string(6) "BACKED"
+  ["private"]=>
+  string(7) "PRIVATE"
+  ["changed"]=>
+  string(12) "CHANGED TEST"
+}
+array(5) {
+  ["addedHooks"]=>
+  string(10) "addedHooks"
+  ["backed"]=>
+  string(6) "backed"
+  ["%0Test%0private"]=>
+  string(7) "private"
+  ["%0Test%0changed"]=>
+  string(12) "changed Test"
+  ["%0Child%0changed"]=>
+  string(13) "changed Child"
+}
+\Child::__set_state(array(
+   'addedHooks' => 'ADDEDHOOKS',
+   'virtual' => 'VIRTUAL',
+   'backed' => 'BACKED',
+   'private' => 'PRIVATE',
+   'changed' => 'CHANGED TEST',
+   'changed' => 'CHANGED CHILD',
+))
+{"addedHooks":"ADDEDHOOKS","virtual":"VIRTUAL","backed":"BACKED"}
+array(5) {
+  ["addedHooks"]=>
+  string(10) "addedHooks"
+  ["backed"]=>
+  string(6) "backed"
+  ["%0Test%0private"]=>
+  string(7) "private"
+  ["%0Test%0changed"]=>
+  string(12) "changed Test"
+  ["%0Child%0changed"]=>
+  string(13) "changed Child"
+}
+
+
+Child::dumpChild():
 object(Child)#%d (5) {
  ["addedHooks"]=>
  string(10) "addedHooks"
@@ -162,73 +236,16 @@ array(4) {
  string(7) "VIRTUAL"
  ["backed"]=>
  string(6) "BACKED"
-  ["private"]=>
-  string(7) "PRIVATE"
-}
-array(5) {
-  ["addedHooks"]=>
-  string(10) "addedHooks"
-  ["backed"]=>
-  string(6) "backed"
-  ["%0Test%0private"]=>
-  string(7) "private"
-  ["%0Test%0changed"]=>
-  string(12) "changed Test"
-  ["%0Child%0changed"]=>
-  string(13) "changed Child"
-}
-\Child::__set_state(array(
-   'addedHooks' => 'ADDEDHOOKS',
-   'changed' => 'CHANGED CHILD',
-   'virtual' => 'VIRTUAL',
-   'backed' => 'BACKED',
-   'private' => 'PRIVATE',
-   'changed' => 'changed Child',
-))
-{"addedHooks":"ADDEDHOOKS","virtual":"VIRTUAL","backed":"BACKED"}
-array(5) {
-  ["addedHooks"]=>
-  string(10) "addedHooks"
-  ["backed"]=>
-  string(6) "backed"
-  ["%0Test%0private"]=>
-  string(7) "private"
-  ["%0Test%0changed"]=>
-  string(12) "changed Test"
-  ["%0Child%0changed"]=>
-  string(13) "changed Child"
-}
-object(Child)#%d (5) {
-  ["addedHooks"]=>
-  string(10) "addedHooks"
-  ["backed"]=>
-  string(6) "backed"
-  ["private":"Test":private]=>
-  string(7) "private"
-  ["changed":"Test":private]=>
-  string(12) "changed Test"
-  ["changed":"Child":private]=>
-  string(13) "changed Child"
-}
-array(5) {
-  ["addedHooks"]=>
-  string(10) "ADDEDHOOKS"
  ["changed"]=>
  string(13) "CHANGED CHILD"
-  ["virtual"]=>
-  string(7) "VIRTUAL"
-  ["backed"]=>
-  string(6) "BACKED"
-  ["changed"]=>
-  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) {
--- a/Zend/tests/property_hooks/foreach.phpt
+++ b/Zend/tests/property_hooks/foreach.phpt
@@ -86,8 +86,35 @@ testByVal(new ByVal);
 testByVal(new ByRef);
 testByRef(new ByRef);

+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) {
+            var_dump($k, $v);
+        }
+    }
+}
+
+#[AllowDynamicProperties]
+class B extends A {
+    public $changed { get => 'B'; }
+    public $promoted { get => 'B'; }
+}
+
+$b = new B;
+$b->shadowed = 'Global';
+$b->test();
+
 ?>
 --EXPECTF--
+plain => plain
+ByRef::$virtualByRef::get
+virtualByRef => virtualByRef
+ByRef::$virtualByRef::set
 ByVal::$virtualByVal::get
 virtualByVal => virtualByVal
 ByVal::$virtualByVal::set
@@ -97,10 +124,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 +164,11 @@ object(ByRef)#%d (3) {
  ["dynamic"]=>
  string(7) "DYNAMIC"
 }
+string(7) "changed"
+string(1) "A"
+string(8) "promoted"
+string(1) "B"
+string(9) "protected"
+string(1) "A"
+string(8) "shadowed"
+string(1) "A"
--- a/Zend/tests/property_hooks/gh16185.phpt
+++ b/Zend/tests/property_hooks/gh16185.phpt
@@ -0,0 +1,28 @@
+--TEST--
+GH-16185: Incorrect indexing into dynamic property array
+--FILE--
+<?php
+
+class ByRef {
+    private $_virtualByRef = 'virtualByRef';
+}
+
+class ByVal extends ByRef {
+    public $_virtualByRef {
+        get => 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
--- a/Zend/tests/property_hooks/gh16185_002.phpt
+++ b/Zend/tests/property_hooks/gh16185_002.phpt
@@ -0,0 +1,30 @@
+--TEST--
+GH-16185: Hooked object iterator with readonly props
+--FILE--
+<?php
+
+class C {
+    public readonly int $prop;
+    public $dummy { set {} }
+
+    public function init() {
+        $this->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
--- a/Zend/zend_property_hooks.c
+++ b/Zend/zend_property_hooks.c
@@ -36,42 +36,69 @@ 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);
+				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);
+				}
+			}
+			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 +120,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 +139,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 +152,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;
@@ -140,16 +175,26 @@ 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);
-
-			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)
@@ -169,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);
 	}
@@ -266,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));
 }
@@ -311,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, 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);