From: Nikita Popov Date: Tue, 12 Jan 2016 15:31:58 +0000 (+0100) Subject: Fix bug #71334 X-Git-Tag: php-7.0.6RC1~49^2~4 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=b1e854f7762b2e0648ddc240835cb446ee4d20a7;p=php 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 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(¶ms[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-- + [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 $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-- + "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" +}