Fix GH-16649: Avoid UAF when using array_splice

NEWS

@@ -73,6 +73,7 @@ PHP                                                                        NEWS
   . Fix theoretical issues with hrtime() not being available. (nielsdos)
   . Fixed bug GH-19300 (Nested array_multisort invocation with error breaks).
     (nielsdos)
+  . Fixed bug GH-16649 (UAF during array_splice). (alexandre-daubois)
 
 - Windows:
   . Free opened_path when opened_path_len >= MAXPATHLEN. (dixyes)

ext/standard/array.c

@@ -3214,6 +3214,9 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
 	zval		*entry;				/* Hash entry */
 	uint32_t    iter_pos = zend_hash_iterators_lower_pos(in_hash, 0);
 
+	GC_ADDREF(in_hash);
+	HT_ALLOW_COW_VIOLATION(in_hash); /* Will be reset when setting the flags for in_hash */
+
 	/* Get number of entries in the input hash */
 	num_in = zend_hash_num_elements(in_hash);
 
@@ -3372,6 +3375,15 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
 	HT_SET_ITERATORS_COUNT(&out_hash, HT_ITERATORS_COUNT(in_hash));
 	HT_SET_ITERATORS_COUNT(in_hash, 0);
 	in_hash->pDestructor = NULL;
+
+	if (UNEXPECTED(GC_DELREF(in_hash) == 0)) {
+		/* Array was completely deallocated during the operation */
+		zend_array_destroy(in_hash);
+		zend_hash_destroy(&out_hash);
+		zend_throw_error(NULL, "Array was modified during array_splice operation");
+		return;
+	}
+
 	zend_hash_destroy(in_hash);
 
 	HT_FLAGS(in_hash)          = HT_FLAGS(&out_hash);

ext/standard/tests/array/gh16649/array_splice_normal_destructor.phpt

@@ -0,0 +1,20 @@
+--TEST--
+GH-16649: array_splice with normal destructor should work fine
+--FILE--
+<?php
+class C {
+    function __destruct() {
+        echo "Destructor called\n";
+    }
+}
+
+$arr = ["1", new C, "2"];
+array_splice($arr, 1, 2);
+var_dump($arr);
+?>
+--EXPECT--
+Destructor called
+array(1) {
+  [0]=>
+  string(1) "1"
+}

ext/standard/tests/array/gh16649/array_splice_uaf_add_elements.phpt

@@ -0,0 +1,22 @@
+--TEST--
+GH-16649: array_splice UAF when destructor adds elements to array
+--FILE--
+<?php
+class C {
+    function __destruct() {
+        global $arr;
+        $arr[] = 0;
+    }
+}
+
+$arr = ["1", new C, "2"];
+
+try {
+    array_splice($arr, 1, 2);
+    echo "ERROR: Should have thrown exception\n";
+} catch (Error $e) {
+    echo "Exception caught: " . $e->getMessage() . "\n";
+}
+?>
+--EXPECT--
+Exception caught: Array was modified during array_splice operation

ext/standard/tests/array/gh16649/array_splice_uaf_array_deallocated.phpt

@@ -0,0 +1,22 @@
+--TEST--
+GH-16649: array_splice UAF when array is released entirely
+--FILE--
+<?php
+class C {
+    function __destruct() {
+        global $arr;
+        $arr = null;
+    }
+}
+
+$arr = ["1", new C, "2"];
+
+try {
+    array_splice($arr, 1, 2);
+    echo "ERROR: Should have thrown exception\n";
+} catch (Error $e) {
+    echo "Exception caught: " . $e->getMessage() . "\n";
+}
+?>
+--EXPECT--
+Exception caught: Array was modified during array_splice operation

ext/standard/tests/array/gh16649/array_splice_uaf_complex_modification.phpt

@@ -0,0 +1,25 @@
+--TEST--
+GH-16649: array_splice UAF with complex array modification
+--FILE--
+<?php
+class ComplexModifier {
+    function __destruct() {
+        global $arr;
+        // complex modification that causes cow
+        unset($arr[0]);
+        $arr["new_key"] = "new_value";
+        $arr[100] = "another_value";
+    }
+}
+
+$arr = ["first", new ComplexModifier, "last"];
+
+try {
+    array_splice($arr, 1, 1, ["replacement"]);
+    echo "ERROR: Should have thrown exception\n";
+} catch (Error $e) {
+    echo "Exception caught: " . $e->getMessage() . "\n";
+}
+?>
+--EXPECT--
+Exception caught: Array was modified during array_splice operation

ext/standard/tests/array/gh16649/array_splice_uaf_multiple_destructors.phpt

@@ -0,0 +1,33 @@
+--TEST--
+GH-16649: array_splice UAF with multiple destructors
+--FILE--
+<?php
+class MultiDestructor {
+    public $id;
+
+    function __construct($id) { 
+        $this->id = $id; 
+    }
+
+    function __destruct() {
+        global $arr;
+        echo "Destructor {$this->id} called\n";
+        if ($this->id == 2) {
+            $arr = null;
+        }
+    }
+}
+
+$arr = ["start", new MultiDestructor(1), new MultiDestructor(2), "end"];
+
+try {
+    array_splice($arr, 1, 2);
+    echo "ERROR: Should have thrown exception\n";
+} catch (Error $e) {
+    echo "Exception caught: " . $e->getMessage() . "\n";
+}
+?>
+--EXPECT--
+Destructor 1 called
+Destructor 2 called
+Exception caught: Array was modified during array_splice operation

ext/standard/tests/array/gh16649/array_splice_uaf_original_case.phpt

@@ -0,0 +1,29 @@
+--TEST--
+GH-16649: array_splice UAF with destructor modifying array (original case)
+--FILE--
+<?php
+function resize_arr() {
+    global $arr;
+    for ($i = 0; $i < 10; $i++) {
+        $arr[$i] = $i;
+    }
+}
+
+class C {
+    function __destruct() {
+        resize_arr();
+        return "3";
+    }
+}
+
+$arr = ["a" => "1", "3" => new C, "2" => "2"];
+
+try {
+    array_splice($arr, 1, 2);
+    echo "ERROR: Should have thrown exception\n";
+} catch (Error $e) {
+    echo "Exception caught: " . $e->getMessage() . "\n";
+}
+?>
+--EXPECT--
+Exception caught: Array was modified during array_splice operation

ext/standard/tests/array/gh16649/array_splice_uaf_packed_to_hash.phpt

@@ -0,0 +1,23 @@
+--TEST--
+GH-16649: array_splice UAF when array is converted from packed to hash
+--FILE--
+<?php
+class C {
+    function __destruct() {
+        global $arr;
+        // array is converted from packed to hash
+        $arr["str"] = 0;
+    }
+}
+
+$arr = ["1", new C, "2"];
+
+try {
+    array_splice($arr, 1, 2);
+    echo "ERROR: Should have thrown exception\n";
+} catch (Error $e) {
+    echo "Exception caught: " . $e->getMessage() . "\n";
+}
+?>
+--EXPECT--
+Exception caught: Array was modified during array_splice operation

ext/standard/tests/array/gh16649/array_splice_with_replacement.phpt

@@ -0,0 +1,23 @@
+--TEST--
+GH-16649: array_splice with replacement array when destructor modifies array
+--FILE--
+<?php
+class C {
+    function __destruct() {
+        global $arr;
+        $arr["modified"] = "by_destructor";
+    }
+}
+
+$arr = ["a", new C, "b"];
+$replacement = ["replacement1", "replacement2"];
+
+try {
+    array_splice($arr, 1, 1, $replacement);
+    echo "ERROR: Should have thrown exception\n";
+} catch (Error $e) {
+    echo "Exception caught: " . $e->getMessage() . "\n";
+}
+?>
+--EXPECT--
+Exception caught: Array was modified during array_splice operation