Skip to content

Commit 076d09e

Browse files
committed
Fix GH-22678: array_multisort() use-after-free on mutating comparator
array_multisort() snapshotted each input array's buckets without holding a reference, then ran a comparator that under SORT_STRING invokes Stringable::__toString(); a callback that unset or reassigned the array freed its backing store and elements mid-sort, a use-after-free read in the comparator and a write-back through the freed table during the restructure. Hold a reference on each input HashTable for the duration of the sort and restructure the cached tables, mirroring zend_array_sort_ex() (GH-16648). HT_ALLOW_COW_VIOLATION allows the in-place repack under the held reference. Fixes GH-22678
1 parent 8b1079a commit 076d09e

3 files changed

Lines changed: 71 additions & 6 deletions

File tree

NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -85,6 +85,8 @@ PHP NEWS
8585
(Weilin Du)
8686
. Fixed integer overflow in getimagesize() and getimagesizefromstring()
8787
when parsing an IFF chunk with a size of INT_MAX. (David Carlier, Weilin Du)
88+
. Fixed bug GH-22678 (Use-after-free in array_multisort() when the comparator
89+
mutates the array being sorted). (azchin, iliaal)
8890

8991
- Zip:
9092
. Fixed bug GH-22649 (ZipArchive::setCommentName() and setCommentIndex()

ext/standard/array.c

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -6046,6 +6046,7 @@ PHP_FUNCTION(array_multisort)
60466046
{
60476047
zval* args;
60486048
zval** arrays;
6049+
HashTable** hashes;
60496050
Bucket** indirect;
60506051
uint32_t idx;
60516052
HashTable* hash;
@@ -6167,20 +6168,26 @@ PHP_FUNCTION(array_multisort)
61676168
for (i = 0; i < array_size; i++) {
61686169
indirect[i] = indirects + (i * (num_arrays + 1));
61696170
}
6171+
hashes = (HashTable **)safe_emalloc(num_arrays, sizeof(HashTable *), 0);
6172+
for (i = 0; i < num_arrays; i++) {
6173+
hashes[i] = Z_ARRVAL_P(arrays[i]);
6174+
GC_ADDREF(hashes[i]);
6175+
HT_ALLOW_COW_VIOLATION(hashes[i]);
6176+
}
61706177
for (i = 0; i < num_arrays; i++) {
61716178
k = 0;
6172-
if (HT_IS_PACKED(Z_ARRVAL_P(arrays[i]))) {
6173-
zval *zv = Z_ARRVAL_P(arrays[i])->arPacked;
6174-
for (idx = 0; idx < Z_ARRVAL_P(arrays[i])->nNumUsed; idx++, zv++) {
6179+
if (HT_IS_PACKED(hashes[i])) {
6180+
zval *zv = hashes[i]->arPacked;
6181+
for (idx = 0; idx < hashes[i]->nNumUsed; idx++, zv++) {
61756182
if (Z_TYPE_P(zv) == IS_UNDEF) continue;
61766183
ZVAL_COPY_VALUE(&indirect[k][i].val, zv);
61776184
indirect[k][i].h = idx;
61786185
indirect[k][i].key = NULL;
61796186
k++;
61806187
}
61816188
} else {
6182-
Bucket *p = Z_ARRVAL_P(arrays[i])->arData;
6183-
for (idx = 0; idx < Z_ARRVAL_P(arrays[i])->nNumUsed; idx++, p++) {
6189+
Bucket *p = hashes[i]->arData;
6190+
for (idx = 0; idx < hashes[i]->nNumUsed; idx++, p++) {
61846191
if (Z_TYPE(p->val) == IS_UNDEF) continue;
61856192
indirect[k][i] = *p;
61866193
k++;
@@ -6200,7 +6207,7 @@ PHP_FUNCTION(array_multisort)
62006207

62016208
/* Restructure the arrays based on sorted indirect - this is mostly taken from zend_hash_sort() function. */
62026209
for (i = 0; i < num_arrays; i++) {
6203-
hash = Z_ARRVAL_P(arrays[i]);
6210+
hash = hashes[i];
62046211
hash->nNumUsed = array_size;
62056212
hash->nNextFreeElement = array_size;
62066213
hash->nInternalPointer = 0;
@@ -6229,6 +6236,14 @@ PHP_FUNCTION(array_multisort)
62296236
RETVAL_TRUE;
62306237

62316238
clean_up:
6239+
for (i = 0; i < num_arrays; i++) {
6240+
if (UNEXPECTED(GC_DELREF(hashes[i]) == 0)) {
6241+
zend_array_destroy(hashes[i]);
6242+
} else {
6243+
gc_check_possible_root((zend_refcounted *)hashes[i]);
6244+
}
6245+
}
6246+
efree(hashes);
62326247
efree(indirects);
62336248
efree(indirect);
62346249
efree(func);
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
--TEST--
2+
GH-22678 (Use-after-free in array_multisort() when the comparator mutates the array)
3+
--FILE--
4+
<?php
5+
class Evil {
6+
public function __toString(): string {
7+
// Runs from the SORT_STRING comparator; drop the array being sorted.
8+
foreach (array_keys($GLOBALS['a']) as $k) {
9+
unset($GLOBALS['a'][$k]);
10+
}
11+
$GLOBALS['a'] = [];
12+
return "x";
13+
}
14+
}
15+
16+
$a = [];
17+
for ($i = 0; $i < 10; $i++) {
18+
$a[] = new Evil();
19+
}
20+
$GLOBALS['a'] = &$a;
21+
22+
echo "freed mid-sort: ";
23+
var_dump(array_multisort($a, SORT_STRING));
24+
unset($GLOBALS['a']);
25+
26+
// Non-packed integer keys reach the mixed-to-packed restructure while the sort
27+
// holds its reference on the array.
28+
$b = [5 => 'c', 3 => 'a', 1 => 'b'];
29+
array_multisort($b, SORT_STRING);
30+
echo "repacked: ", implode(',', $b), "\n";
31+
32+
class Boom {
33+
public function __toString(): string {
34+
throw new Exception("boom");
35+
}
36+
}
37+
38+
$c = [new Boom(), new Boom()];
39+
try {
40+
array_multisort($c, SORT_STRING);
41+
} catch (Exception $e) {
42+
echo $e::class, ": ", $e->getMessage(), "\n";
43+
}
44+
?>
45+
--EXPECT--
46+
freed mid-sort: bool(true)
47+
repacked: a,b,c
48+
Exception: boom

0 commit comments

Comments
 (0)