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.

diff --git a/NEWS b/NEWS
index abf70d04c1ffa14b775ee2941f3f1a0133335044..b8e4eefe090780c1b628c2434f83021347880279 100644 (file)
--- 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 d5cb32606ea4cb4843548ed2f50dbbb3c174976c..60cbac572697ead5ca180c976cda09928d94005d 100644 (file)
--- 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 4ff373a0b0265cf9bf82a3bb9a22b239c98b2a1c..289870ac1f2b323a2e36485071c7ce6752369e28 100644 (file)
--- 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 (file)
index 0000000..7a37d09
--- /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 58f2249205410d8fd62694c31ee5b18d7f4d2ed0..2762ebd8309477ed11e4ffb2f348a3e12fcbd13d 100644 (file)
--- 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 (file)
index 0000000..b5a1ee2
--- /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"
+}