From 10c2160ffb26dc28d5b85abd3d502f65f895b895 Mon Sep 17 00:00:00 2001 From: Denis Vaksman Date: Mon, 8 Aug 2022 21:48:17 +0300 Subject: [PATCH 1/4] Fix GH-9296: `ksort` behaves incorrectly on arrays with string and numeric keys The comparator function used at ksort in SORT_REGULAR mode need to be consistent with basic comparison rules. These rules were changed in PHP-8.0 for numeric strings, but comparator used at ksort kept the old behaviour. It leads to inconsistent situations, when after ksort the first key is GREATER than some of the next ones by according to the basic comparison operators. --- ext/standard/array.c | 48 +++++++++++++------------------------------- 1 file changed, 14 insertions(+), 34 deletions(-) diff --git a/ext/standard/array.c b/ext/standard/array.c index 32ad1b0eaa6e..be44aa28837b 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -166,40 +166,20 @@ static zend_never_inline ZEND_COLD int stable_sort_fallback(Bucket *a, Bucket *b static zend_always_inline int php_array_key_compare_unstable_i(Bucket *f, Bucket *s) /* {{{ */ { - zend_uchar t; - zend_long l1, l2; - double d; - - if (f->key == NULL) { - if (s->key == NULL) { - return (zend_long)f->h > (zend_long)s->h ? 1 : -1; - } else { - l1 = (zend_long)f->h; - t = is_numeric_string(s->key->val, s->key->len, &l2, &d, 1); - if (t == IS_LONG) { - /* pass */ - } else if (t == IS_DOUBLE) { - return ZEND_NORMALIZE_BOOL((double)l1 - d); - } else { - l2 = 0; - } - } - } else { - if (s->key) { - return zendi_smart_strcmp(f->key, s->key); - } else { - l2 = (zend_long)s->h; - t = is_numeric_string(f->key->val, f->key->len, &l1, &d, 1); - if (t == IS_LONG) { - /* pass */ - } else if (t == IS_DOUBLE) { - return ZEND_NORMALIZE_BOOL(d - (double)l2); - } else { - l1 = 0; - } - } - } - return ZEND_NORMALIZE_BOOL(l1 - l2); + zval first; + zval second; + + if (f->key) { + ZVAL_STR(&first, f->key); + } else { + ZVAL_LONG(&first, f->h); + } + if (s->key) { + ZVAL_STR(&second, s->key); + } else { + ZVAL_LONG(&second, s->h); + } + return zend_compare(&first, &second); } /* }}} */ From 5c4ef9ab92390d5c6f327d4f2070f84a24cdf6b6 Mon Sep 17 00:00:00 2001 From: Denis Vaksman Date: Tue, 9 Aug 2022 17:33:22 +0300 Subject: [PATCH 2/4] Fix ksort tests to make expected results consistent with php8 comparison rules --- ext/standard/tests/array/002.phpt | 20 +++++++++--------- .../tests/array/krsort_variation8.phpt | Bin 3121 -> 3121 bytes .../tests/array/krsort_variation9.phpt | 8 +++---- .../tests/array/ksort_variation8.phpt | Bin 3107 -> 3107 bytes .../tests/array/ksort_variation9.phpt | 8 +++---- 5 files changed, 18 insertions(+), 18 deletions(-) diff --git a/ext/standard/tests/array/002.phpt b/ext/standard/tests/array/002.phpt index fdb838deb19d..5cf377e2a353 100644 --- a/ext/standard/tests/array/002.phpt +++ b/ext/standard/tests/array/002.phpt @@ -276,6 +276,8 @@ array(8) { -- Testing krsort() -- No second argument: array(8) { + ["test"]=> + int(27) [16777216]=> float(-0.3333333333333333) [1001]=> @@ -288,8 +290,6 @@ array(8) { string(4) "Test" [0]=> string(3) "PHP" - ["test"]=> - int(27) [-1000]=> array(2) { [0]=> @@ -300,6 +300,8 @@ array(8) { } Using SORT_REGULAR: array(8) { + ["test"]=> + int(27) [16777216]=> float(-0.3333333333333333) [1001]=> @@ -312,8 +314,6 @@ array(8) { string(4) "Test" [0]=> string(3) "PHP" - ["test"]=> - int(27) [-1000]=> array(2) { [0]=> @@ -334,10 +334,10 @@ array(8) { string(27) "PHP: Hypertext Preprocessor" [5]=> string(4) "Test" - [0]=> - string(3) "PHP" ["test"]=> int(27) + [0]=> + string(3) "PHP" [-1000]=> array(2) { [0]=> @@ -383,8 +383,6 @@ array(8) { } [0]=> string(3) "PHP" - ["test"]=> - int(27) [5]=> string(4) "Test" [17]=> @@ -395,6 +393,8 @@ array(8) { string(6) "monkey" [16777216]=> float(-0.3333333333333333) + ["test"]=> + int(27) } Using SORT_REGULAR: array(8) { @@ -407,8 +407,6 @@ array(8) { } [0]=> string(3) "PHP" - ["test"]=> - int(27) [5]=> string(4) "Test" [17]=> @@ -419,6 +417,8 @@ array(8) { string(6) "monkey" [16777216]=> float(-0.3333333333333333) + ["test"]=> + int(27) } Using SORT_NUMERIC: array(8) { diff --git a/ext/standard/tests/array/krsort_variation8.phpt b/ext/standard/tests/array/krsort_variation8.phpt index 1483e6fd1bbc18abe1c129d73043254e66ee6dc7..39e72e2eb22211e4f3bb45963fc3ae7c3dab99d7 100644 GIT binary patch delta 107 zcmdleu~A}!DBEUdHhZSYqHKDTk8+z%7UGeRR8WXkQi`>;<5Ey4E-A{)OV=>aR8Ugl Zs-3)w)orpKrxu15d7N6fT=Mf$xd8tI8>j#P delta 113 zcmdleu~A}!DBI)^4!zByZ1zl(k8+z%7UdC@QBa81HHx*h<5Eya%gIkH(a<&0v$Oz0 bO|II>t61G8>v3vf=u*O`i_0ZHFO>@bu$dh& diff --git a/ext/standard/tests/array/krsort_variation9.phpt b/ext/standard/tests/array/krsort_variation9.phpt index 4364911b6212..b0309f373d36 100644 --- a/ext/standard/tests/array/krsort_variation9.phpt +++ b/ext/standard/tests/array/krsort_variation9.phpt @@ -82,22 +82,22 @@ array(5) { - With default sort flag - bool(true) array(3) { + ["c"]=> + string(5) "apple" ["a"]=> string(6) "orange" [0]=> string(6) "banana" - ["c"]=> - string(5) "apple" } - Sort flag = SORT_REGULAR - bool(true) array(3) { + ["c"]=> + string(5) "apple" ["a"]=> string(6) "orange" [0]=> string(6) "banana" - ["c"]=> - string(5) "apple" } -- Iteration 3 -- diff --git a/ext/standard/tests/array/ksort_variation8.phpt b/ext/standard/tests/array/ksort_variation8.phpt index 7b7e1c003e587897379970a72be8a1936fb5fce1..9a15c4dc3e75d7dd76ec29986cc84159096b381c 100644 GIT binary patch delta 91 zcmZ21u~=dQ2ixRSPKn80oHCQ^IAtd<*K`?!raf9G7y$XGjh9;@qQ R1x_u3O4)04FgRDC8f!goJx})aK>#e=32=prl1gQ w08x{clb=|kVW4MVV5m9Sg3EI9CQfa}+R5`+T_-DWYEi>9_F68N{Jc~y069b~q5uE@ diff --git a/ext/standard/tests/array/ksort_variation9.phpt b/ext/standard/tests/array/ksort_variation9.phpt index 9322af70f57f..0023a9e0602a 100644 --- a/ext/standard/tests/array/ksort_variation9.phpt +++ b/ext/standard/tests/array/ksort_variation9.phpt @@ -81,20 +81,20 @@ array(5) { - With default sort flag - bool(true) array(3) { - ["a"]=> - string(6) "orange" [0]=> string(6) "banana" + ["a"]=> + string(6) "orange" ["c"]=> string(5) "apple" } - Sort flag = SORT_REGULAR - bool(true) array(3) { - ["a"]=> - string(6) "orange" [0]=> string(6) "banana" + ["a"]=> + string(6) "orange" ["c"]=> string(5) "apple" } From aa125c8f848920dbf11cac10ca69f44ec2f65773 Mon Sep 17 00:00:00 2001 From: Denis Vaksman Date: Thu, 11 Aug 2022 18:01:36 +0300 Subject: [PATCH 3/4] Review fixes --- ext/standard/array.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/ext/standard/array.c b/ext/standard/array.c index be44aa28837b..d145661e3782 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -169,6 +169,11 @@ static zend_always_inline int php_array_key_compare_unstable_i(Bucket *f, Bucket zval first; zval second; + if (f->key == NULL && s->key == NULL) { + return (zend_long)f->h > (zend_long)s->h ? 1 : -1; + } else if (f->key && s->key) { + return zendi_smart_strcmp(f->key, s->key); + } if (f->key) { ZVAL_STR(&first, f->key); } else { From 23fc6d43b9cda1296776d0687988b09c6c0faa30 Mon Sep 17 00:00:00 2001 From: Denis Vaksman Date: Thu, 11 Aug 2022 18:12:13 +0300 Subject: [PATCH 4/4] Add the test case for GH-9296 --- ext/standard/tests/array/gh9296.phpt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 ext/standard/tests/array/gh9296.phpt diff --git a/ext/standard/tests/array/gh9296.phpt b/ext/standard/tests/array/gh9296.phpt new file mode 100644 index 000000000000..cfbc5cbbb09e --- /dev/null +++ b/ext/standard/tests/array/gh9296.phpt @@ -0,0 +1,17 @@ +--TEST-- +GH-9296: incorrect ksort(..., SORT_REGULAR) behaviour on arrays with numeric and string keys +--FILE-- + 0, 600 => 1]; +ksort($array, SORT_REGULAR); +var_dump($array); +var_dump(array_key_first($array) <=> array_key_last($array)); +?> +--EXPECT-- +array(2) { + [600]=> + int(1) + ["aaa"]=> + int(0) +} +int(-1)