Fix GH-10519: Array Data Address Reference Issue

NathanFreeman · Girgias · commit 49b2ff5dbb94 · 2023-03-10T14:23:30.000Z
We need to carry around a reference to the underlying Bucket to be able to modify it by reference. Closes GH-10749 Signed-off-by: George Peter Banyard <girgias@php.net>
diff --git a/NEWS b/NEWS
@@ -25,6 +25,9 @@ PHP                                                                        NEWS
   . Add missing error checks on EVP_MD_CTX_create() and EVP_VerifyInit().
     (nielsdos)
 
+- SPL:
+  . Fixed bug GH-10519 (Array Data Address Reference Issue). (Nathan Freeman)
+
 16 Mar 2023, PHP 8.1.17
 
 - Core:
diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
@@ -29,7 +29,6 @@
 #include "php_spl.h"
 #include "spl_array_arginfo.h"
 #include "spl_functions.h"
-#include "spl_engine.h"
 #include "spl_iterators.h"
 #include "spl_array.h"
 #include "spl_exceptions.h"
@@ -63,6 +62,8 @@ typedef struct _spl_array_object {
 	uint32_t          ht_iter;
 	int               ar_flags;
 	unsigned char	  nApplyCount;
+	bool			  is_child;
+	Bucket			  *bucket;
 	zend_function     *fptr_offset_get;
 	zend_function     *fptr_offset_set;
 	zend_function     *fptr_offset_has;
@@ -167,6 +168,8 @@ static zend_object *spl_array_object_new_ex(zend_class_entry *class_type, zend_o
 	object_properties_init(&intern->std, class_type);
 
 	intern->ar_flags = 0;
+	intern->is_child = false;
+	intern->bucket = NULL;
 	intern->ce_get_iterator = spl_ce_ArrayIterator;
 	if (orig) {
 		spl_array_object *other = spl_array_from_obj(orig);
@@ -477,6 +480,22 @@ static zval *spl_array_read_dimension(zend_object *object, zval *offset, int typ
 	return spl_array_read_dimension_ex(1, object, offset, type, rv);
 } /* }}} */
 
+/*
+ * The assertion(HT_ASSERT_RC1(ht)) failed because the refcount was increased manually when intern->is_child is true.
+ * We have to set the refcount to 1 to make assertion success and restore the refcount to the original value after
+ * modifying the array when intern->is_child is true.
+ */
+static uint32_t spl_array_set_refcount(bool is_child, HashTable *ht, uint32_t refcount) /* {{{ */
+{
+	uint32_t old_refcount = 0;
+	if (is_child) {
+		old_refcount = GC_REFCOUNT(ht);
+		GC_SET_REFCOUNT(ht, refcount);
+	}
+
+	return old_refcount;
+} /* }}} */
+
 static void spl_array_write_dimension_ex(int check_inherited, zend_object *object, zval *offset, zval *value) /* {{{ */
 {
 	spl_array_object *intern = spl_array_from_obj(object);
@@ -500,9 +519,16 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
 	}
 
 	Z_TRY_ADDREF_P(value);
+
+	uint32_t refcount = 0;
 	if (!offset || Z_TYPE_P(offset) == IS_NULL) {
 		ht = spl_array_get_hash_table(intern);
+		refcount = spl_array_set_refcount(intern->is_child, ht, 1);
 		zend_hash_next_index_insert(ht, value);
+
+		if (refcount) {
+			spl_array_set_refcount(intern->is_child, ht, refcount);
+		}
 		return;
 	}
 
@@ -513,12 +539,17 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec
 	}
 
 	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);
 		spl_hash_key_release(&key);
 	} else {
 		zend_hash_index_update(ht, key.h, value);
 	}
+
+	if (refcount) {
+		spl_array_set_refcount(intern->is_child, ht, refcount);
+	}
 } /* }}} */
 
 static void spl_array_write_dimension(zend_object *object, zval *offset, zval *value) /* {{{ */
@@ -548,6 +579,8 @@ static void spl_array_unset_dimension_ex(int check_inherited, zend_object *objec
 	}
 
 	ht = spl_array_get_hash_table(intern);
+	uint32_t refcount = spl_array_set_refcount(intern->is_child, ht, 1);
+
 	if (key.key) {
 		zval *data = zend_hash_find(ht, key.key);
 		if (data) {
@@ -570,6 +603,10 @@ static void spl_array_unset_dimension_ex(int check_inherited, zend_object *objec
 	} else {
 		zend_hash_index_del(ht, key.h);
 	}
+
+	if (refcount) {
+		spl_array_set_refcount(intern->is_child, ht, refcount);
+	}
 } /* }}} */
 
 static void spl_array_unset_dimension(zend_object *object, zval *offset) /* {{{ */
@@ -1058,6 +1095,16 @@ static void spl_array_set_array(zval *object, spl_array_object *intern, zval *ar
 		} else {
 			//??? TODO: try to avoid array duplication
 			ZVAL_ARR(&intern->array, zend_array_dup(Z_ARR_P(array)));
+
+			if (intern->is_child) {
+				Z_TRY_DELREF_P(&intern->bucket->val);
+				/*
+				 * replace bucket->val with copied array, so the changes between
+				 * parent and child object can affect each other.
+				 */
+				intern->bucket->val = intern->array;
+				Z_TRY_ADDREF_P(&intern->array);
+			}
 		}
 	} else {
 		if (Z_OBJ_HT_P(array) == &spl_handler_ArrayObject || Z_OBJ_HT_P(array) == &spl_handler_ArrayIterator) {
@@ -1544,6 +1591,22 @@ PHP_METHOD(RecursiveArrayIterator, hasChildren)
 }
 /* }}} */
 
+static void spl_instantiate_child_arg(zend_class_entry *pce, zval *retval, zval *arg1, zval *arg2) /* {{{ */
+{
+	object_init_ex(retval, pce);
+	spl_array_object *new_intern = Z_SPLARRAY_P(retval);
+	/*
+	 * set new_intern->is_child is true to indicate that the object was created by
+	 * RecursiveArrayIterator::getChildren() method.
+	 */
+	new_intern->is_child = true;
+
+	/* find the bucket of parent object. */
+	new_intern->bucket = (Bucket *)((char *)(arg1) - XtOffsetOf(Bucket, val));;
+	zend_call_known_instance_method_with_2_params(pce->constructor, Z_OBJ_P(retval), NULL, arg1, arg2);
+}
+/* }}} */
+
 /* {{{ Create a sub iterator for the current element (same class as $this) */
 PHP_METHOD(RecursiveArrayIterator, getChildren)
 {
@@ -1574,7 +1637,7 @@ PHP_METHOD(RecursiveArrayIterator, getChildren)
 	}
 
 	ZVAL_LONG(&flags, intern->ar_flags);
-	spl_instantiate_arg_ex2(Z_OBJCE_P(ZEND_THIS), return_value, entry, &flags);
+	spl_instantiate_child_arg(Z_OBJCE_P(ZEND_THIS), return_value, entry, &flags);
 }
 /* }}} */
 
diff --git a/ext/spl/tests/bug42654.phpt b/ext/spl/tests/bug42654.phpt
@@ -110,11 +110,11 @@ object(RecursiveArrayIterator)#%d (1) {
     [2]=>
     array(2) {
       [2]=>
-      string(4) "val2"
+      string(5) "alter"
       [3]=>
       array(1) {
         [3]=>
-        string(4) "val3"
+        string(5) "alter"
       }
     }
     [4]=>
@@ -129,11 +129,11 @@ object(RecursiveArrayIterator)#%d (1) {
     [2]=>
     array(2) {
       [2]=>
-      string(4) "val2"
+      string(5) "alter"
       [3]=>
       array(1) {
         [3]=>
-        string(4) "val3"
+        string(5) "alter"
       }
     }
     [4]=>
@@ -146,11 +146,11 @@ array(3) {
   [2]=>
   array(2) {
     [2]=>
-    string(4) "val2"
+    string(5) "alter"
     [3]=>
     array(1) {
       [3]=>
-      string(4) "val3"
+      string(5) "alter"
     }
   }
   [4]=>
diff --git a/ext/spl/tests/bug42654_2.phpt b/ext/spl/tests/bug42654_2.phpt
@@ -0,0 +1,33 @@
+--TEST--
+Bug #42654 (RecursiveIteratorIterator modifies only part of leaves)
+--FILE--
+<?php
+$data = array ('key1' => 'val1', array('key2' => 'val2'), 'key3' => 'val3');
+
+$iterator = new RecursiveIteratorIterator(new RecursiveArrayIterator($data));
+foreach($iterator as $foo) {
+    $key = $iterator->key();
+    switch($key) {
+        case 'key1': // first level
+        case 'key2': // recursive level
+            echo "update $key".PHP_EOL;
+            $iterator->offsetSet($key, 'alter');
+    }
+}
+$copy = $iterator->getArrayCopy();
+var_dump($copy);
+?>
+--EXPECT--
+update key1
+update key2
+array(3) {
+  ["key1"]=>
+  string(5) "alter"
+  [0]=>
+  array(1) {
+    ["key2"]=>
+    string(5) "alter"
+  }
+  ["key3"]=>
+  string(4) "val3"
+}
diff --git a/ext/spl/tests/gh10519.phpt b/ext/spl/tests/gh10519.phpt
@@ -0,0 +1,69 @@
+--TEST--
+Bug GH-10519 (Array Data Address Reference Issue)
+--FILE--
+<?php
+interface DataInterface extends JsonSerializable, RecursiveIterator, Iterator
+{
+    public static function init(Traversable $data): DataInterface;
+}
+
+class A extends RecursiveArrayIterator implements DataInterface
+{
+    public function __construct($data)
+    {
+        parent::__construct($data);
+    }
+
+    public static function init($data): DataInterface
+    {
+        return new static($data);
+    }
+
+    public function getCols(string $colname): array
+    {
+        return array_column($this->getArrayCopy(), $colname);
+    }
+
+    public function bugySetMethod($key, $value)
+    {
+        $data = &$this;
+
+        while ($data->hasChildren()) {
+            $data = $data->getChildren();
+        }
+	    $data->offsetSet($key, $value);
+    }
+
+    public function jsonSerialize(): mixed
+    {
+        return $this;
+    }
+}
+
+$a = [
+    'test' => [
+        'a' => (object) [2 => '',3 => '',4 => ''],
+    ]
+];
+
+$example = A::init($a);
+$example->bugySetMethod(5, 'in here');
+var_dump(json_encode($example));
+var_dump(json_encode($a));
+
+$b = [
+    'test' => [
+        'b' => [2 => '',3 => '',4 => ''],
+    ]
+];
+$example = A::init($b);
+
+$example->bugySetMethod(5, 'must be here');
+var_dump(json_encode($example));
+var_dump(json_encode($b));
+?>
+--EXPECT--
+string(51) "{"test":{"a":{"2":"","3":"","4":"","5":"in here"}}}"
+string(51) "{"test":{"a":{"2":"","3":"","4":"","5":"in here"}}}"
+string(56) "{"test":{"b":{"2":"","3":"","4":"","5":"must be here"}}}"
+string(37) "{"test":{"b":{"2":"","3":"","4":""}}}"
