Fix GH-11972: RecursiveCallbackFilterIterator regression in 8.1.18

When you do an assignment between two zvals (no, not zval*), you copy all fields. This includes the additional u2 data. So that means for example the Z_NEXT index gets copied, which in some cases can therefore cause a cycle in zend_hash lookups. Instead of doing an assignment, we should be doing a ZVAL_COPY (or ZVAL_COPY_VALUE for non-refcounting cases). This avoids copying u2. Closes GH-12086.
php · Aug 30, 2023 · 1cdcbc0 · 1cdcbc0
1 parent fb0f421
commit 1cdcbc0
Show file tree

Hide file tree

Showing 3 changed files with 202 additions and 3 deletions.
diff --git a/NEWS b/NEWS
@@ -18,6 +18,10 @@ PHP                                                                        NEWS
   . Fixed bug GH-10270 (Invalid error message when connection via SSL fails:
     "trying to connect via (null)"). (Kamil Tekiela)
 
+- SPL:
+  . Fixed bug GH-11972 (RecursiveCallbackFilterIterator regression in 8.1.18).
+    (nielsdos)
+
 31 Aug 2023, PHP 8.1.23
 
 - CLI:

diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
@@ -1128,13 +1128,12 @@ static void spl_array_set_array(zval *object, spl_array_object *intern, zval *ar
 			ZVAL_ARR(&intern->array, zend_array_dup(Z_ARR_P(array)));
 
 			if (intern->is_child) {
-				Z_TRY_DELREF_P(&intern->bucket->val);
+				Z_TRY_DELREF(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);
+				ZVAL_COPY(&intern->bucket->val, &intern->array);
 			}
 		}
 	} else {

diff --git a/ext/spl/tests/gh11972.phpt b/ext/spl/tests/gh11972.phpt
@@ -0,0 +1,196 @@
+--TEST--
+GH-11972 (RecursiveCallbackFilterIterator regression in 8.1.18)
+--EXTENSIONS--
+spl
+--FILE--
+<?php
+
+class RecursiveFilterTest {
+    public function traverse(array $variables): array {
+        $array_iterator = new \RecursiveArrayIterator($variables);
+        $filter_iterator = new \RecursiveCallbackFilterIterator($array_iterator, [
+            $this, 'isCyclic',
+        ]);
+        $recursive_iterator = new \RecursiveIteratorIterator($filter_iterator, \RecursiveIteratorIterator::SELF_FIRST);
+        $recursive_iterator->setMaxDepth(20);
+        foreach ($recursive_iterator as $value) {
+            // Avoid recursion by marking where we've been.
+            $value['#override_mode_breadcrumb'] = true;
+        }
+        return \iterator_to_array($recursive_iterator);
+    }
+
+    public function isCyclic($current, string $key, \RecursiveArrayIterator $iterator): bool {
+        var_dump($current);
+        if (!is_array($current)) {
+            return false;
+        }
+        // Avoid infinite loops by checking if we've been here before.
+        // e.g. View > query > view > query ...
+        if (isset($current['#override_mode_breadcrumb'])) {
+            return false;
+        }
+        return true;
+    }
+}
+
+$test_array['e']['p'][] = ['a', 'a'];
+$test_array['e']['p'][] = ['b', 'b'];
+$test_array['e']['p'][] = ['c', 'c'];
+$serialized = serialize($test_array);
+$unserialized = unserialize($serialized);
+
+$test_class = new RecursiveFilterTest();
+$test_class->traverse($unserialized);
+
+echo "Done\n";
+
+?>
+--EXPECT--
+array(1) {
+  ["p"]=>
+  array(3) {
+    [0]=>
+    array(2) {
+      [0]=>
+      string(1) "a"
+      [1]=>
+      string(1) "a"
+    }
+    [1]=>
+    array(2) {
+      [0]=>
+      string(1) "b"
+      [1]=>
+      string(1) "b"
+    }
+    [2]=>
+    array(2) {
+      [0]=>
+      string(1) "c"
+      [1]=>
+      string(1) "c"
+    }
+  }
+}
+array(3) {
+  [0]=>
+  array(2) {
+    [0]=>
+    string(1) "a"
+    [1]=>
+    string(1) "a"
+  }
+  [1]=>
+  array(2) {
+    [0]=>
+    string(1) "b"
+    [1]=>
+    string(1) "b"
+  }
+  [2]=>
+  array(2) {
+    [0]=>
+    string(1) "c"
+    [1]=>
+    string(1) "c"
+  }
+}
+array(2) {
+  [0]=>
+  string(1) "a"
+  [1]=>
+  string(1) "a"
+}
+string(1) "a"
+string(1) "a"
+array(2) {
+  [0]=>
+  string(1) "b"
+  [1]=>
+  string(1) "b"
+}
+string(1) "b"
+string(1) "b"
+array(2) {
+  [0]=>
+  string(1) "c"
+  [1]=>
+  string(1) "c"
+}
+string(1) "c"
+string(1) "c"
+array(1) {
+  ["p"]=>
+  array(3) {
+    [0]=>
+    array(2) {
+      [0]=>
+      string(1) "a"
+      [1]=>
+      string(1) "a"
+    }
+    [1]=>
+    array(2) {
+      [0]=>
+      string(1) "b"
+      [1]=>
+      string(1) "b"
+    }
+    [2]=>
+    array(2) {
+      [0]=>
+      string(1) "c"
+      [1]=>
+      string(1) "c"
+    }
+  }
+}
+array(3) {
+  [0]=>
+  array(2) {
+    [0]=>
+    string(1) "a"
+    [1]=>
+    string(1) "a"
+  }
+  [1]=>
+  array(2) {
+    [0]=>
+    string(1) "b"
+    [1]=>
+    string(1) "b"
+  }
+  [2]=>
+  array(2) {
+    [0]=>
+    string(1) "c"
+    [1]=>
+    string(1) "c"
+  }
+}
+array(2) {
+  [0]=>
+  string(1) "a"
+  [1]=>
+  string(1) "a"
+}
+string(1) "a"
+string(1) "a"
+array(2) {
+  [0]=>
+  string(1) "b"
+  [1]=>
+  string(1) "b"
+}
+string(1) "b"
+string(1) "b"
+array(2) {
+  [0]=>
+  string(1) "c"
+  [1]=>
+  string(1) "c"
+}
+string(1) "c"
+string(1) "c"
+Done