ext/spl: Fix various ArrayObject bugs

UPGRADING

@@ -59,6 +59,17 @@ PHP 8.4 UPGRADE NOTES
     OutOfBoundsException instead of RuntimeException. As OutOfBoundsException
     is a child class of RuntimeException, code that uses RuntimeException
     continues to function.
+  . ArrayObject now follows property write restrictions.
+    This means, writing a dynamic property now emits a deprecation warning,
+    it is not possible to assign a value of an incorrect type to a typed
+    property, nor assigning to a property name which contains null bytes.
+    ArrayObject continues to bypass any __set() handler and continues to
+    allow assigning to inaccessible property names (e.g. integer as name).
+  . Calling ArrayObject::offsetSet() with an offset of type null will now
+    set an offset instead of appending.
+  . Instances of classes that extend ArrayObject now call append($value)
+    instead of offsetSet(null, $value) when using the append operator
+    e.g. $object[] = $value;
 
 - Standard:
   . round() now validates the value of the $mode parameter and throws a ValueError

ext/spl/spl_array.c

@@ -49,10 +49,14 @@ typedef struct _spl_array_object {
 	unsigned char	  nApplyCount;
 	bool			  is_child;
 	Bucket			  *bucket;
+	/* Overridden ArrayAccess methods */
 	zend_function     *fptr_offset_get;
 	zend_function     *fptr_offset_set;
 	zend_function     *fptr_offset_has;
 	zend_function     *fptr_offset_del;
+	/* Overridden append() method */
+	zend_function     *fptr_append;
+	/* Overridden count() method */
 	zend_function     *fptr_count;
 	zend_class_entry* ce_get_iterator;
 	zend_object       std;
@@ -192,6 +196,7 @@ static zend_object *spl_array_object_new_ex(zend_class_entry *class_type, zend_o
 	ZEND_ASSERT(parent);
 
 	if (inherited) {
+		/* Find potentially overridden ArrayAccess methods */
 		intern->fptr_offset_get = zend_hash_str_find_ptr(&class_type->function_table, "offsetget", sizeof("offsetget") - 1);
 		if (intern->fptr_offset_get->common.scope == parent) {
 			intern->fptr_offset_get = NULL;
@@ -208,7 +213,12 @@ static zend_object *spl_array_object_new_ex(zend_class_entry *class_type, zend_o
 		if (intern->fptr_offset_del->common.scope == parent) {
 			intern->fptr_offset_del = NULL;
 		}
-		/* Find count() method */
+		/* Find potentially overridden append() method */
+		intern->fptr_append = zend_hash_str_find_ptr(&class_type->function_table, "append",  sizeof("append") - 1);
+		if (intern->fptr_append->common.scope == parent) {
+			intern->fptr_append = NULL;
+		}
+		/* Find potentially overridden count() method */
 		intern->fptr_count = zend_hash_find_ptr(&class_type->function_table, ZSTR_KNOWN(ZEND_STR_COUNT));
 		if (intern->fptr_count->common.scope == parent) {
 			intern->fptr_count = NULL;
@@ -460,32 +470,41 @@ static uint32_t spl_array_set_refcount(bool is_child, HashTable *ht, uint32_t re
 	return old_refcount;
 } /* }}} */
 
-static void spl_array_write_dimension_ex(int check_inherited, zend_object *object, zval *offset, zval *value) /* {{{ */
+static void spl_array_write_dimension_ex(bool check_inherited, zend_object *object, zval *offset, zval *value) /* {{{ */
 {
 	spl_array_object *intern = spl_array_from_obj(object);
 	HashTable *ht;
 	spl_hash_key key;
-
-	if (check_inherited && intern->fptr_offset_set) {
-		zval tmp;
-
-		if (!offset) {
-			ZVAL_NULL(&tmp);
-			offset = &tmp;
-		}
-		zend_call_method_with_2_params(object, object->ce, &intern->fptr_offset_set, "offsetSet", NULL, offset, value);
-		return;
-	}
+	uint32_t refcount = 0;
 
 	if (intern->nApplyCount > 0) {
 		zend_throw_error(NULL, "Modification of ArrayObject during sorting is prohibited");
 		return;
 	}
 
-	Z_TRY_ADDREF_P(value);
+	/* We are appending */
+	if (!offset) {
+		/* append() method is overridden, so call it */
+		if (check_inherited && intern->fptr_append) {
+			zend_call_known_function(
+				intern->fptr_append,
+				object,
+				object->ce,
+				/* retval_ptr */NULL,
+				/* param_count */ 1,
+				/* params */ value,
+				/* named_params */ NULL
+			);
+			return;
+		}
 
-	uint32_t refcount = 0;
-	if (!offset || Z_TYPE_P(offset) == IS_NULL) {
+		/* Cannot append if backing value is an object */
+		if (spl_array_is_object(intern)) {
+			zend_throw_error(NULL, "Cannot append properties to objects, use %s::offsetSet() instead", ZSTR_VAL(object->ce->name));
+			return;
+		}
+
+		Z_TRY_ADDREF_P(value);
 		ht = spl_array_get_hash_table(intern);
 		refcount = spl_array_set_refcount(intern->is_child, ht, 1);
 		zend_hash_next_index_insert(ht, value);
@@ -496,18 +515,48 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
 		return;
 	}
 
+	/* offsetSet() method is overridden, so call it */
+	if (check_inherited && intern->fptr_offset_set) {
+		zend_call_method_with_2_params(object, object->ce, &intern->fptr_offset_set, "offsetSet", NULL, offset, value);
+		return;
+	}
+
 	if (get_hash_key(&key, intern, offset) == FAILURE) {
 		zend_illegal_container_offset(object->ce->name, offset, BP_VAR_W);
-		zval_ptr_dtor(value);
 		return;
 	}
 
 	ht = spl_array_get_hash_table(intern);
 	refcount = spl_array_set_refcount(intern->is_child, ht, 1);
 	if (key.key) {
-		zend_hash_update_ind(ht, key.key, value);
+		if (spl_array_is_object(intern) && !(intern->ar_flags & SPL_ARRAY_IS_SELF)) {
+			ZEND_ASSERT(Z_TYPE(intern->array) == IS_OBJECT);
+			zend_object *obj = Z_OBJ(intern->array);
+			/* For reasons ArrayObject must not go through the overloaded __set()/__get()/etc.
+			 * magic methods, so to emit a warning for dynamic props (or Error on classes
+			 * that do not support them, such as readonly classes) or an error for incompatible
+			 * typed properties, we set a property guard so that when calling the write_property
+			 * handler it does not call the magic __set() method. Thanks SPL. */
+			if (obj->ce->ce_flags & ZEND_ACC_USE_GUARDS) {
+				uint32_t *guard = zend_get_property_guard(obj, key.key);
+				uint32_t backup = *guard;
+				(*guard) |= ZEND_GUARD_PROPERTY_SET;
+				obj->handlers->write_property(obj, key.key, value, /* cache_slot */ NULL);
+				*guard = backup;
+			} else {
+				/* No overload method is defined on the object */
+				obj->handlers->write_property(obj, key.key, value, /* cache_slot */ NULL);
+			}
+		} else {
+			/* Increase refcount for value before adding to the Hashtable */
+			Z_TRY_ADDREF_P(value);
+			zend_hash_update_ind(ht, key.key, value);
+		}
 		spl_hash_key_release(&key);
 	} else {
+		/* Increase refcount for value before adding to the Hashtable */
+		Z_TRY_ADDREF_P(value);
+		ZEND_ASSERT(!spl_array_is_object(intern));
 		zend_hash_index_update(ht, key.h, value);
 	}
 
@@ -518,7 +567,7 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
 
 static void spl_array_write_dimension(zend_object *object, zval *offset, zval *value) /* {{{ */
 {
-	spl_array_write_dimension_ex(1, object, offset, value);
+	spl_array_write_dimension_ex(/* check_inherited */ true, object, offset, value);
 } /* }}} */
 
 static void spl_array_unset_dimension_ex(int check_inherited, zend_object *object, zval *offset) /* {{{ */
@@ -679,18 +728,12 @@ PHP_METHOD(ArrayObject, offsetSet)
 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "zz", &index, &value) == FAILURE) {
 		RETURN_THROWS();
 	}
-	spl_array_write_dimension_ex(0, Z_OBJ_P(ZEND_THIS), index, value);
+	spl_array_write_dimension_ex(/* check_inherited */ false, Z_OBJ_P(ZEND_THIS), index, value);
 } /* }}} */
 
+/* Needed for spl_iterators.c:2938 */
 void spl_array_iterator_append(zval *object, zval *append_value) /* {{{ */
 {
-	spl_array_object *intern = Z_SPLARRAY_P(object);
-
-	if (spl_array_is_object(intern)) {
-		zend_throw_error(NULL, "Cannot append properties to objects, use %s::offsetSet() instead", ZSTR_VAL(Z_OBJCE_P(object)->name));
-		return;
-	}
-
 	spl_array_write_dimension(Z_OBJ_P(object), NULL, append_value);
 } /* }}} */
 
@@ -702,7 +745,7 @@ PHP_METHOD(ArrayObject, append)
 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &value) == FAILURE) {
 		RETURN_THROWS();
 	}
-	spl_array_iterator_append(ZEND_THIS, value);
+	spl_array_write_dimension_ex(/* check_inherited */ false, Z_OBJ_P(ZEND_THIS), /* offset */ NULL, value);
 } /* }}} */
 
 /* {{{ Unsets the value at the specified $index. */

ext/spl/tests/ArrayObject/appending_object.phpt

@@ -0,0 +1,28 @@
+--TEST--
+Cannot append to ArrayObject if backing value is an object
+--EXTENSIONS--
+spl
+--FILE--
+<?php
+class MyClass {}
+$c = new MyClass();
+
+$ao = new ArrayObject($c);
+
+try {
+    $ao->append('no-key');
+    var_dump($c);
+} catch (\Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+
+try {
+    $ao[] = 'no-key';
+    var_dump($c);
+} catch (\Throwable $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+?>
+--EXPECT--
+Error: Cannot append properties to objects, use ArrayObject::offsetSet() instead
+Error: Cannot append properties to objects, use ArrayObject::offsetSet() instead

ext/spl/tests/bug33136.phpt → ext/spl/tests/ArrayObject/bug33136.phpt

@@ -9,7 +9,7 @@ class Collection extends ArrayObject
 
     function __construct()
     {
-        $this->data = array();
+        $this->data = [];
         parent::__construct($this->data);
     }
 
@@ -26,7 +26,7 @@ class Collection extends ArrayObject
     }
 }
 
-echo "\n\nInitiate Obj\n";
+echo "Initiate Obj\n";
 $arrayObj = new Collection();
 
 echo "Assign values\n";
@@ -41,37 +41,35 @@ var_dump($arrayObj[1]);
 $arrayObj["foo"] = "baz";
 var_dump($arrayObj["foo"]);
 
-print_r($arrayObj);
+var_dump($arrayObj);
 
 var_dump(count($arrayObj));
 
 ?>
 --EXPECT--
 Initiate Obj
 Assign values
-Collection::offsetSet(NULL,foo)
 Collection::offsetGet(0)
 string(3) "foo"
-Collection::offsetSet(NULL,bar)
 Collection::offsetGet(0)
 string(3) "foo"
 Collection::offsetGet(1)
 string(3) "bar"
 Collection::offsetSet(foo,baz)
 Collection::offsetGet(foo)
 string(3) "baz"
-Collection Object
-(
-    [data:Collection:private] => Array
-        (
-        )
-
-    [storage:ArrayObject:private] => Array
-        (
-            [0] => foo
-            [1] => bar
-            [foo] => baz
-        )
-
-)
+object(Collection)#1 (2) {
+  ["data":"Collection":private]=>
+  array(0) {
+  }
+  ["storage":"ArrayObject":private]=>
+  array(3) {
+    [0]=>
+    string(3) "foo"
+    [1]=>
+    string(3) "bar"
+    ["foo"]=>
+    string(3) "baz"
+  }
+}
 int(3)

ext/spl/tests/ArrayObject/bug34548.phpt

@@ -0,0 +1,39 @@
+--TEST--
+Bug #34548 (Method append() in class extended from ArrayObject crashes PHP)
+--FILE--
+<?php
+
+class Collection extends ArrayObject
+{
+    public function add($dataArray)
+    {
+        foreach ($dataArray as $value) {
+            $this->append($value);
+        }
+    }
+}
+
+$data1 = ['one', 'two', 'three'];
+$data2 = ['four', 'five'];
+
+$foo = new Collection($data1);
+$foo->add($data2);
+
+var_dump($foo->getArrayCopy());
+
+echo "Done\n";
+?>
+--EXPECT--
+array(5) {
+  [0]=>
+  string(3) "one"
+  [1]=>
+  string(3) "two"
+  [2]=>
+  string(5) "three"
+  [3]=>
+  string(4) "four"
+  [4]=>
+  string(4) "five"
+}
+Done

ext/spl/tests/ArrayObject/custom_append.phpt

@@ -0,0 +1,42 @@
+--TEST--
+Overwritting default append method
+--EXTENSIONS--
+spl
+--FILE--
+<?php
+
+class Dummy {}
+
+class OverWriteAppend extends ArrayObject
+{
+    private $data;
+
+    function __construct($v)
+    {
+        $this->data = [];
+        parent::__construct($v);
+    }
+
+    function append($value): void
+    {
+        $this->data[] = $value;
+    }
+}
+
+$d = new Dummy();
+$ao = new OverWriteAppend($d);
+
+$ao->append("value1");
+var_dump($ao);
+?>
+--EXPECT--
+object(OverWriteAppend)#2 (2) {
+  ["data":"OverWriteAppend":private]=>
+  array(1) {
+    [0]=>
+    string(6) "value1"
+  }
+  ["storage":"ArrayObject":private]=>
+  object(Dummy)#1 (0) {
+  }
+}