GSCArray: fix out-of-bounds access in GSCArrayQuickSort#47
Conversation
After partitioning, the pivot ends up at index "stored", so the left partition holds "stored" elements ([0, stored-1]) and the right partition holds "length - stored - 1" elements ([stored+1, length-1]). The recursive calls passed the wrong lengths: GSCArrayQuickSort (array, stored - 1, ...); /* misses one */ GSCArrayQuickSort (array + stored + 1, length - stored, ...); /* one too many */ The right call's length was one too large, so the sub-sort accessed array[length] -- one past the end of the array. With a CFArray sorted in place this is an out-of-bounds read (AddressSanitizer: heap-buffer-overflow in GSCArrayInsertionSort, via GSCArrayQuickSort, for a 17-element array). The left call's length was one too small, leaving an element unsorted by the recursion. Pass "stored" and "length - stored - 1". Adds a regression test that sorts more than 16 values.
| /* The pivot ends up at index "stored", so the left partition has | ||
| "stored" elements and the right partition has the remaining | ||
| "length - stored - 1" (passing length - stored read one past the | ||
| end of the array). */ |
| PASS_CF(n == 5, "Index of value between values is %d.", (int)n); | ||
|
|
||
|
|
||
| { |
There was a problem hiding this comment.
Let's use a randomly shuffled range [0; n] as the test input data, and test for a few different values of n.
There was a problem hiding this comment.
Done in f457ca8. The test now sorts a fixed-seed Fisher–Yates shuffle of a contiguous range for sizes 1,2,5,15,16,17,33,64,100 (straddling the insertion-sort/quicksort threshold) and asserts the exact ascending result each time.
One note: I build the input via a pre-sized mutable copy instead of incremental appends, because growing a mutable CFArray across its capacity boundary trips a separate out-of-bounds write in CFArrayCheckCapacityAndGrow/CFArrayReplaceValues (unrelated to this fix). Happy to file that separately.
There was a problem hiding this comment.
Correction on the note above: that CFArray grow-path overflow is not unreported — it is already fixed in #45 (recompute the destination pointer after CFArrayCheckCapacityAndGrow reallocs, and grow to at least the requested capacity). I re-root-caused it here (use-after-realloc write in CFArrayReplaceValues) and confirmed #45 resolves it under ASan. This test just pre-sizes to keep the two PRs independent.
There was a problem hiding this comment.
Does the test pass on CoreFoundation on MacOS?
There was a problem hiding this comment.
Someone else will have to test - I have no access to Mac hardware.
Drop the explanatory comment in GSCArrayQuickSort, and rewrite the sort test to sort a randomly shuffled permutation of a contiguous range for several sizes that straddle the 16-element insertion-sort / quicksort threshold, asserting the exact ascending result each time. The shuffle uses a fixed seed so failures reproduce. The input array is built with a pre-sized mutable copy rather than incremental appends: growing a mutable CFArray across its capacity boundary trips a separate out-of-bounds write in CFArrayCheckCapacityAndGrow / CFArrayReplaceValues, which is unrelated to this fix and would otherwise mask the quicksort behaviour.
3 participants
After partitioning, the pivot ends up at index "stored", so the left
partition holds "stored" elements ([0, stored-1]) and the right partition
holds "length - stored - 1" elements ([stored+1, length-1]). The
recursive calls passed the wrong lengths:
GSCArrayQuickSort (array, stored - 1, ...); /* misses one /
GSCArrayQuickSort (array + stored + 1, length - stored, ...); / one too many */
The right call's length was one too large, so the sub-sort accessed
array[length] -- one past the end of the array. With a CFArray sorted in
place this is an out-of-bounds read (AddressSanitizer: heap-buffer-overflow
in GSCArrayInsertionSort, via GSCArrayQuickSort, for a 17-element array).
The left call's length was one too small, leaving an element unsorted by
the recursion.
Pass "stored" and "length - stored - 1". Adds a regression test that
sorts more than 16 values.