Fix GH-22678: array_multisort() use-after-free on mutating comparator#22680
Fix GH-22678: array_multisort() use-after-free on mutating comparator#22680iliaal wants to merge 1 commit into
Conversation
|
it seems almost co-authored by the issue reporter no ? |
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() (phpGH-16648). HT_ALLOW_COW_VIOLATION allows the in-place repack under the held reference. Fixes phpGH-22678
iliaal
force-pushed
the
fix/gh-22678-multisort-uaf
branch
from
87034b5 to
076d09e
Compare
|
The reporter's issue included a proposed patch, so the approach follows it (and the |
| 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]); |
There was a problem hiding this comment.
Array may be interned I believe, so you should use GC_TRY_ADDREF
There was a problem hiding this comment.
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.
| for (i = 0; i < num_arrays; i++) { | ||
| hashes[i] = Z_ARRVAL_P(arrays[i]); | ||
| GC_ADDREF(hashes[i]); | ||
| HT_ALLOW_COW_VIOLATION(hashes[i]); |
There was a problem hiding this comment.
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.
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
Okay but I'm not a fan of the complication
| for (i = 0; i < array_size; i++) { | ||
| indirect[i] = indirects + (i * (num_arrays + 1)); | ||
| } | ||
| hashes = (HashTable **)safe_emalloc(num_arrays, sizeof(HashTable *), 0); |
3 participants
array_multisort() borrows each input array's buckets without a refcount, then runs the comparator; under SORT_STRING that reaches Stringable::__toString(), so a callback that unsets or reassigns the array frees its backing store and elements mid-sort (use-after-free in the comparator, and a write-back through the freed table in the restructure). Holding a reference on each input HashTable for the duration of the sort fixes it, mirroring zend_array_sort_ex() (GH-16648); HT_ALLOW_COW_VIOLATION lets the in-place restructure repack the still-referenced table. Fixes #22678