Skip to content

Fix GH-22678: array_multisort() use-after-free on mutating comparator #22680

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iliaal wants to merge 1 commit into
base: PHP-8.4
Choose a base branch
from
+71 −6
Open

Fix GH-22678: array_multisort() use-after-free on mutating comparator #22680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,8 @@ PHP NEWS
(Weilin Du)
. Fixed integer overflow in getimagesize() and getimagesizefromstring()
when parsing an IFF chunk with a size of INT_MAX. (David Carlier, Weilin Du)
. Fixed bug GH-22678 (Use-after-free in array_multisort() when the comparator
mutates the array being sorted). (azchin, iliaal)

- Zip:
. Fixed bug GH-22649 (ZipArchive::setCommentName() and setCommentIndex()
Expand Down
27 changes: 21 additions & 6 deletions ext/standard/array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6046,6 +6046,7 @@ PHP_FUNCTION(array_multisort)
{
zval* args;
zval** arrays;
HashTable** hashes;
Bucket** indirect;
uint32_t idx;
HashTable* hash;
Expand Down Expand Up @@ -6167,20 +6168,26 @@ PHP_FUNCTION(array_multisort)
for (i = 0; i < array_size; i++) {
indirect[i] = indirects + (i * (num_arrays + 1));
}
hashes = (HashTable **)safe_emalloc(num_arrays, sizeof(HashTable *), 0);
for (i = 0; i < num_arrays; i++) {
hashes[i] = Z_ARRVAL_P(arrays[i]);
GC_ADDREF(hashes[i]);

@ndossche ndossche Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array may be interned I believe, so you should use GC_TRY_ADDREF

@iliaal iliaal Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hashes[i] is separated to a mutable array before this (SEPARATE_ARRAY in the parse loop) and restructured in place afterwards, so it's never interned here. An interned class-const array passed in sorts a separated copy and leaves the original intact (checked under opcache.enable_cli). This matches the plain GC_ADDREF in zend_array_sort_ex.

HT_ALLOW_COW_VIOLATION(hashes[i]);

@ndossche ndossche Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No

@iliaal iliaal Jul 10, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same array, so it's mutable here too. The flag is needed because the restructure's zend_hash_to_packed asserts RC1 under the held reference; php_splice() sets HT_ALLOW_COW_VIOLATION right after its own GC_ADDREF for the same reason.

}
for (i = 0; i < num_arrays; i++) {
k = 0;
if (HT_IS_PACKED(Z_ARRVAL_P(arrays[i]))) {
zval *zv = Z_ARRVAL_P(arrays[i])->arPacked;
for (idx = 0; idx < Z_ARRVAL_P(arrays[i])->nNumUsed; idx++, zv++) {
if (HT_IS_PACKED(hashes[i])) {
zval *zv = hashes[i]->arPacked;
for (idx = 0; idx < hashes[i]->nNumUsed; idx++, zv++) {
if (Z_TYPE_P(zv) == IS_UNDEF) continue;
ZVAL_COPY_VALUE(&indirect[k][i].val, zv);
indirect[k][i].h = idx;
indirect[k][i].key = NULL;
k++;
}
} else {
Bucket *p = Z_ARRVAL_P(arrays[i])->arData;
for (idx = 0; idx < Z_ARRVAL_P(arrays[i])->nNumUsed; idx++, p++) {
Bucket *p = hashes[i]->arData;
for (idx = 0; idx < hashes[i]->nNumUsed; idx++, p++) {
if (Z_TYPE(p->val) == IS_UNDEF) continue;
indirect[k][i] = *p;
k++;
Expand All @@ -6200,7 +6207,7 @@ PHP_FUNCTION(array_multisort)

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

clean_up:
for (i = 0; i < num_arrays; i++) {
if (UNEXPECTED(GC_DELREF(hashes[i]) == 0)) {
zend_array_destroy(hashes[i]);
} else {
gc_check_possible_root((zend_refcounted *)hashes[i]);
}
}
efree(hashes);
efree(indirects);
efree(indirect);
efree(func);
Expand Down
48 changes: 48 additions & 0 deletions ext/standard/tests/array/gh22678.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
--TEST--
GH-22678 (Use-after-free in array_multisort() when the comparator mutates the array)
--FILE--
<?php
class Evil {
public function __toString(): string {
// Runs from the SORT_STRING comparator; drop the array being sorted.
foreach (array_keys($GLOBALS['a']) as $k) {
unset($GLOBALS['a'][$k]);
}
$GLOBALS['a'] = [];
return "x";
}
}

$a = [];
for ($i = 0; $i < 10; $i++) {
$a[] = new Evil();
}
$GLOBALS['a'] = &$a;

echo "freed mid-sort: ";
var_dump(array_multisort($a, SORT_STRING));
unset($GLOBALS['a']);

// Non-packed integer keys reach the mixed-to-packed restructure while the sort
// holds its reference on the array.
$b = [5 => 'c', 3 => 'a', 1 => 'b'];
array_multisort($b, SORT_STRING);
echo "repacked: ", implode(',', $b), "\n";

class Boom {
public function __toString(): string {
throw new Exception("boom");
}
}

$c = [new Boom(), new Boom()];
try {
array_multisort($c, SORT_STRING);
} catch (Exception $e) {
echo $e::class, ": ", $e->getMessage(), "\n";
}
?>
--EXPECT--
freed mid-sort: bool(true)
repacked: a,b,c
Exception: boom
Loading