Fix bug #71334

Always duplicate the array before doing a sort with user-defined
comparison function, to avoid access to the intermediate
inconsistent state.

I've also dropped the "array modification" warning, as protection
against modifications is no longer relevant if we're always working
on a copy anyway.

This also required some changes to how SplArray forwards calls to
sorting functions.

6 files changed, 130 insertions, 41 deletions

diff --git a/NEWS b/NEWS
index abf70d04c1..b8e4eefe09 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,7 @@ PHP                                                                        NEWS
   . Fixed bug #62059 (ArrayObject and isset are not friends). (Nikita)
   . Fixed bug #71871 (Interfaces allow final and abstract functions). (Nikita)
   . Fixed bug #71922 (Crash on assert(new class{})). (Nikita)
+  . Fixed bug #71334 (Cannot access array keys while uksort()). (Nikita)
 
 - Curl:
   . Fixed bug #71831 (CURLOPT_NOPROXY applied as long instead of string).
diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c
index d5cb32606e..60cbac5726 100644
--- a/ext/spl/spl_array.c
+++ b/ext/spl/spl_array.c
@@ -82,18 +82,18 @@ static inline spl_array_object *spl_array_from_obj(zend_object *obj) /* {{{ */ {
 
 #define Z_SPLARRAY_P(zv)  spl_array_from_obj(Z_OBJ_P((zv)))
 
-static inline HashTable *spl_array_get_hash_table(spl_array_object* intern) { /* {{{ */
+static inline HashTable **spl_array_get_hash_table_ptr(spl_array_object* intern) { /* {{{ */
 	//??? TODO: Delay duplication for arrays; only duplicate for write operations
 	if (intern->ar_flags & SPL_ARRAY_IS_SELF) {
 		if (!intern->std.properties) {
 			rebuild_object_properties(&intern->std);
 		}
-		return intern->std.properties;
+		return &intern->std.properties;
 	} else if (intern->ar_flags & SPL_ARRAY_USE_OTHER) {
 		spl_array_object *other = Z_SPLARRAY_P(&intern->array);
-		return spl_array_get_hash_table(other);
+		return spl_array_get_hash_table_ptr(other);
 	} else if (Z_TYPE(intern->array) == IS_ARRAY) {
-		return Z_ARRVAL(intern->array);
+		return &Z_ARRVAL(intern->array);
 	} else {
 		zend_object *obj = Z_OBJ(intern->array);
 		if (!obj->properties) {
@@ -104,9 +104,22 @@ static inline HashTable *spl_array_get_hash_table(spl_array_object* intern) { /*
 			}
 			obj->properties = zend_array_dup(obj->properties);
 		}
-		return obj->properties;
+		return &obj->properties;
 	}
-} /* }}} */
+}
+/* }}} */
+
+static inline HashTable *spl_array_get_hash_table(spl_array_object* intern) { /* {{{ */
+	return *spl_array_get_hash_table_ptr(intern);
+}
+/* }}} */
+
+static inline void spl_array_replace_hash_table(spl_array_object* intern, HashTable *ht) { /* {{{ */
+	HashTable **ht_ptr = spl_array_get_hash_table_ptr(intern);
+	zend_array_destroy(*ht_ptr);
+	*ht_ptr = ht;
+}
+/* }}} */
 
 static inline zend_bool spl_array_is_object(spl_array_object *intern) /* {{{ */
 {
@@ -1432,16 +1445,12 @@ static void spl_array_method(INTERNAL_FUNCTION_PARAMETERS, char *fname, int fnam
 	spl_array_object *intern = Z_SPLARRAY_P(getThis());
 	HashTable *aht = spl_array_get_hash_table(intern);
 	zval function_name, params[2], *arg = NULL;
-	uint32_t old_refcount;
 
 	ZVAL_STRINGL(&function_name, fname, fname_len);
 
-	/* A tricky way to pass "aht" by reference, reset refcount */
-	//??? It may be not safe, if user comparison handler accesses "aht"
-	old_refcount = GC_REFCOUNT(aht);
-	GC_REFCOUNT(aht) = 1;
 	ZVAL_NEW_EMPTY_REF(&params[0]);
 	ZVAL_ARR(Z_REFVAL(params[0]), aht);
+	GC_REFCOUNT(aht)++;
 
 	if (!use_arg) {
 		intern->nApplyCount++;
@@ -1470,10 +1479,16 @@ static void spl_array_method(INTERNAL_FUNCTION_PARAMETERS, char *fname, int fnam
 	}
 
 exit:
-	/* A tricky way to pass "aht" by reference, copy back and cleanup */
-	GC_REFCOUNT(aht) = old_refcount;
-	efree(Z_REF(params[0]));
-	zend_string_free(Z_STR(function_name));
+	{
+		HashTable *new_ht = Z_ARRVAL_P(Z_REFVAL(params[0]));
+		if (aht != new_ht) {
+			spl_array_replace_hash_table(intern, new_ht);
+		} else {
+			GC_REFCOUNT(aht)--;
+		}
+		efree(Z_REF(params[0]));
+		zend_string_free(Z_STR(function_name));
+	}
 } /* }}} */
 
 #define SPL_ARRAY_METHOD(cname, fname, use_arg) \
diff --git a/ext/standard/array.c b/ext/standard/array.c
index 4ff373a0b0..289870ac1f 100644
--- a/ext/standard/array.c
+++ b/ext/standard/array.c
@@ -1035,37 +1035,30 @@ static int php_array_user_compare(const void *a, const void *b) /* {{{ */
 static void php_usort(INTERNAL_FUNCTION_PARAMETERS, compare_func_t compare_func, zend_bool renumber) /* {{{ */
 {
 	zval *array;
-	zend_refcounted *arr;
+	zend_array *arr;
 	zend_bool retval;
 	PHP_ARRAY_CMP_FUNC_VARS;
 
 	PHP_ARRAY_CMP_FUNC_BACKUP();
 
-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "a/f", &array, &BG(user_compare_fci), &BG(user_compare_fci_cache)) == FAILURE) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "af", &array, &BG(user_compare_fci), &BG(user_compare_fci_cache)) == FAILURE) {
 		PHP_ARRAY_CMP_FUNC_RESTORE();
 		return;
 	}
 
-	/* Increase reference counter, so the attempts to modify the array in user
-	 * comparison function will create a copy of array and won't affect the
-	 * original array. The fact of modification is detected by comparing the
-	 * zend_array pointer. The result of sorting in such case is undefined and
-	 * the function returns FALSE.
-	 */
-	Z_ADDREF_P(array);
-	arr = Z_COUNTED_P(array);
+	arr = Z_ARR_P(array);
+	if (zend_hash_num_elements(arr) == 0)  {
+		PHP_ARRAY_CMP_FUNC_RESTORE();
+		RETURN_TRUE;
+	}
 
-	retval = zend_hash_sort(Z_ARRVAL_P(array), compare_func, renumber) != FAILURE;
+	/* Copy array, so the in-place modifications will not be visible to the callback function */
+	arr = zend_array_dup(arr);
 
-	if (arr != Z_COUNTED_P(array)) {
-		php_error_docref(NULL, E_WARNING, "Array was modified by the user comparison function");
-		if (--GC_REFCOUNT(arr) <= 0) {
-			_zval_dtor_func(arr ZEND_FILE_LINE_CC);
-		}
-		retval = 0;
-	} else {
-		Z_DELREF_P(array);
-	}
+	retval = zend_hash_sort(arr, compare_func, renumber) != FAILURE;
+
+	zval_ptr_dtor(array);
+	ZVAL_ARR(array, arr);
 
 	PHP_ARRAY_CMP_FUNC_RESTORE();
 	RETURN_BOOL(retval);
diff --git a/ext/standard/tests/array/bug71334.phpt b/ext/standard/tests/array/bug71334.phpt
new file mode 100644
index 0000000000..7a37d0953a
--- /dev/null
+++ b/ext/standard/tests/array/bug71334.phpt
@@ -0,0 +1,38 @@
+--TEST--
+Bug #71334: Cannot access array keys while uksort()
+--FILE--
+<?php
+
+class myClass
+{
+	private $a = [
+		'foo-test' => [1],
+		'-' => [2],
+		'bar-test' => [3]
+	];
+
+	private function _mySort($x, $y)
+	{
+		if (!isset($this->a[$x])) {
+			throw new Exception('Missing X: "' . $x . '"');
+		}
+
+		if (!isset($this->a[$y])) {
+			throw new Exception('Missing Y: "' . $y . '"');
+		}
+
+		return $x < $y;
+	}
+
+	public function __construct()
+	{
+		uksort($this->a, [$this, '_mySort']);
+	}
+}
+
+new myClass();
+echo "Done";
+
+?>
+--EXPECT--
+Done
diff --git a/ext/standard/tests/array/unexpected_array_mod_bug.phpt b/ext/standard/tests/array/unexpected_array_mod_bug.phpt
index 58f2249205..2762ebd830 100644
--- a/ext/standard/tests/array/unexpected_array_mod_bug.phpt
+++ b/ext/standard/tests/array/unexpected_array_mod_bug.phpt
@@ -4,7 +4,7 @@ Crash when function parameter modified via reference
 <?php
 function usercompare($a,$b) {
   unset($GLOBALS['my_var'][2]); 
-  return 0;
+  return $a <=> $b;
 }
 $my_var = array(1 => "entry_1",
 2 => "entry_2",
@@ -12,10 +12,19 @@ $my_var = array(1 => "entry_1",
 4 => "entry_4",
 5 => "entry_5");
 usort($my_var, "usercompare");
+var_dump($my_var);
 
-echo "Done.\n";
 ?>
---EXPECTF--
-
-Warning: usort(): Array was modified by the user comparison function in %s on line %d
-Done.
+--EXPECT--
+array(5) {
+  [0]=>
+  string(7) "entry_1"
+  [1]=>
+  string(7) "entry_2"
+  [2]=>
+  string(7) "entry_3"
+  [3]=>
+  string(7) "entry_4"
+  [4]=>
+  string(7) "entry_5"
+}
diff --git a/ext/standard/tests/array/unexpected_array_mod_bug_variation1.phpt b/ext/standard/tests/array/unexpected_array_mod_bug_variation1.phpt
new file mode 100644
index 0000000000..b5a1ee24d5
--- /dev/null
+++ b/ext/standard/tests/array/unexpected_array_mod_bug_variation1.phpt
@@ -0,0 +1,33 @@
+--TEST--
+Crash when function parameter modified via reference while keeping orig refcount
+--FILE--
+<?php
+
+$array = array(
+    1 => "entry_1",
+    2 => "entry_2",
+    3 => "entry_3",
+    4 => "entry_4",
+    5 => "entry_5"
+);
+usort($array, function($a, $b) use (&$array, &$ref) {
+    unset($array[2]);
+    $ref = $array;
+    return $a <=> $b;
+});
+var_dump($array);
+
+?>
+--EXPECT--
+array(5) {
+  [0]=>
+  string(7) "entry_1"
+  [1]=>
+  string(7) "entry_2"
+  [2]=>
+  string(7) "entry_3"
+  [3]=>
+  string(7) "entry_4"
+  [4]=>
+  string(7) "entry_5"
+}