com php-src: Fix bug #71334: NEWS ext/spl/spl_array.c ext /standard/array.c ext/standard/tests/array/bug7133 4.phpt
ext/standard/tests/array/unexpected_array_mod _bug.phpt ext/standard/tests/array/unexpected_array_m od_bug_variation1.phpt

Commit: b1e854f7762b2e0648ddc240835cb446ee4d20a7 Author: Nikita Popov <nikic@php.net> Tue, 12 Jan 2016 16:31:58 +0100 Parents: 1866ab2d1e1dfb8ada2414fea373c32b8a195eec Branches: PHP-7.0 master Link: http://git.php.net/?p=php-src.git;a=commitdiff;h=b1e854f7762b2e0648ddc240835cb446ee4d20a7 Log: 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. Bugs: https://bugs.php.net/71334 Changed paths: M NEWS M ext/spl/spl_array.c M ext/standard/array.c A ext/standard/tests/array/bug71334.phpt M ext/standard/tests/array/unexpected_array_mod_bug.phpt A ext/standard/tests/array/unexpected_array_mod_bug_variation1.phpt

diff --git a/NEWS b/NEWS index abf70d0..b8e4eef 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 d5cb326..60cbac5 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 4ff373a..289870a 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 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 58f2249..2762ebd 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 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" +}