Fix various hooked object iterator issues

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) {

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"

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

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);