Add bounds checks in SDL_qsort

[aikawayataro]

Description

Updating qsort implementation fixed only part of the non-transitive compare issue.
Using such a compare function should be considered a user code bug, but I believe it's better not to crash the whole program.
I set up a fuzzer and found a few unchecked memory reads and writes. With the proposed changes, qsort will not crash with invalid compare functions.
Fuzzer source code: fuzzer.zip

[madebr]

These checks are insufficient.
When adding extra tests to testqsort.c (madebr@1ea94b0), ci fails:
https://github.com/madebr/SDL/actions/runs/9597768806/job/26467538455

Feel free to use the commit in this pr to verify the fixes.

[aikawayataro]

@madebr You're accessing a zero-sized allocated block at line 61 when arraylen=0 😁 there's no memory corruptions from qsort.
As for your test of non-transitive compare, this usecase, which is considered invalid and should not be tested.

[DanielGibson]

As for your test of non-transitive compare, this usecase, which is considered invalid and should not be tested.

It's not valid, but testing it to make sure it at least doesn't crash makes sense (that's what these changes are about, after all).
But you can't expect the data to be sorted afterwards, so verifying any order doesn't make sense (even the "incorrect" order after running qsort with invalid comparator on a given array might not be deterministic in the long term, when the qsort implementation is updated again; and it might even be different depending on wordsize and whatever potentially platform-dependent special cases that implementation handles).

(No idea what exactly goes wrong in the qsort test, unless I missed something the log only mentions an incorrect exit code?)

[madebr]

@madebr You're accessing a zero-sized allocated block at line 61 when arraylen=0 😁 there's no memory corruptions from qsort. As for your test of non-transitive compare, this usecase, which is considered invalid and should not be tested.

Whoops! Good catch.
I think my changes still make sense, with a fix for the bug you noticed.

    if (arraylen > 0) {
        prev = nums[0];
    }

[aikawayataro]

It's not valid, but testing it to make sure it at least doesn't crash makes sense (that's what these changes are about, after all).
But you can't expect the data to be sorted afterwards, so verifying any order doesn't make sense (even the "incorrect" order after running qsort with invalid comparator on a given array might not be deterministic in the long term, when the qsort implementation is updated again; and it might even be different depending on wordsize and whatever potentially platform-dependent special cases that implementation handles).

That's what I meant to say, we shouldn't test the order, just the function.

(No idea what exactly goes wrong in the qsort test, unless I missed something the log only mentions an incorrect exit code?)

Because of a bug in the test itself, there's a segfault.

I think my changes still make sense, with a fix for the bug you noticed.

Test runs just fine with your fix (there was a note about a non-existent bug I found, apologies).
Also your arraylen with invalid compare function is too big because it takes crazy amounts of cpu compared to test with valid one. I guess I will add other arraylen values that will cover the whole qsort without very large values. We should also test aligned and unaligned branches. I'll add a test with these remarks in mind.

[aikawayataro]

I refactored the test, but honestly I think it does not look reasonable. The qsort used is in fact 3 qsorts for different cases. To test them all requires a lot of hackery (ignore failing build I can't figure out right pointer type for const array pointer).
What I believe is that we should not use qsort_aligned and qsort_words but use only plain unaligned version, which works in any case.

[slouken]

What's the status of this PR? Is it something we still need?

[aikawayataro]

@slouken
The current state is more like a draft. I've introduced tests to test all three implementations of sorting, but it looks crude to me.
What I propose is to get rid of two other qsort implementations and keep only qsort_r_nonaligned. It will be easier to test a single implementation, and I really don't see reason to keep qsort_r_aligned and qsort_r_words.

[slouken]

@slouken The current state is more like a draft. I've introduced tests to test all three implementations of sorting, but it looks crude to me. What I propose is to get rid of two other qsort implementations and keep only qsort_r_nonaligned. It will be easier to test a single implementation, and I really don't see reason to keep qsort_r_aligned and qsort_r_words.

I would tend to agree, but before we do that, we should test performance in release mode on a modern processor to see if we get significant speedup from those modes.

[aikawayataro]

Ok, I will set up a benchmark for this

[aikawayataro]

I benchmarked it and here are the results:

gcc 14.2.1 -O3
items=5000
rounds=50000
================================
qsort_r_words vs qsort_r_nonaligned for int
qsort_r_words took 171652
qsort_r_nonaligned took 179765
diff=8113, 8.113000ms
qsort_r_words is faster
================================
qsort_r_aligned vs qsort_r_nonaligned for big_struct sizeof=128
qsort_r_aligned took 213069
qsort_r_nonaligned took 202701
diff=10368, 10.368000ms
qsort_r_nonaligned is faster

gcc 14.2.1 -O2
items=5000
rounds=50000
================================
qsort_r_words vs qsort_r_nonaligned for int
qsort_r_words took 165439
qsort_r_nonaligned took 217981
diff=52542, 52.542000ms
qsort_r_words is faster
================================
qsort_r_aligned vs qsort_r_nonaligned for big_struct sizeof=128
qsort_r_aligned took 332536
qsort_r_nonaligned took 874643
diff=542107, 542.107000ms
qsort_r_aligned is faster

CPU: 12th Gen Intel(R) Core(TM) i7-12700H

Well, it makes things harder, I guess.
It can be observed that the nonaligned version performs better under O3 than the aligned version, but it is much slower under O2 (>x2 slower). words version is slightly faster than the nonaligned version in both cases.

I only compared these 2 pairs because
qsort_r_words only useful when size of item is sizeof(int) and when items buffer aligned as int
qsort_r_aligned only useful when item size is sizeof(X) % sizeof(int) == 0 and items buffer aligned as int (almost always due to padding)
qsort_r_nonaligned will always work
So it narrows everything down to these two "competing" cases.

Benchmark code bench.c.tar.gz

[slouken]

So it sounds like it's worthwhile keeping all 3 cases. Thanks for the investigation!

[madebr]

Hey @aikawayataro !

In #14344, I've refactored the tests a bit such that it tests SDL_qsort on:

• int arrays (element size == WORD_BYTES)
• double and int pointer arrays (element size == multiple of WORD_BYTES)
• 3-byte sized struct (element size != multiple of WORD_BYTES)

I've also split up the test in various unit tests using macros.

So far so good, In this branch, I've implemented unit tests for "bad" compare callbacks that are non-transitive and random, which fail (=abort) with the current SDL_qsort implementation.

Would you be willing to have another go at this?

Note:
if you check out the qsort-exotic-compare-callbacks branch, you can build testqsort using SDL_qsort and qsort from your libc (I've tested it against glibc):

cc ../test/testqsort.c -I../include libSDL3_test.a libSDL3.so -lunwind -Wl,-rpath,$PWD -otestqsort_stdlib -DTEST_STDLIB_QSORT
cc ../test/testqsort.c -I../include libSDL3_test.a libSDL3.so -lunwind -Wl,-rpath,$PWD -otestqsort_sdl

testqsort_stdlib succeeds on my system, testqsort_sdl currently fails...

[aikawayataro]

Hello! Yes, I'm still interested in this. I skimmed through 14344 and it does look good, all three qsort branches would be tested.
I think I can reset this PR to cad2dd8 as it contains the change regarding bounds check, and the latter commit that refactors testqsort is redundant, your PR looks much cleaner.

[aikawayataro]

@madebr I checked qsort-exotic-compare-callbacks with this PR, and it indeed fails test with non-transitive compare function (but does not segfault).
I have a question, what output should we expect with non-transitive compare functions? I didn't think about this, because the initial goal for this PR was to prevent segfault. I believe result depends on implementation, so I don't know if you can compare SDL qsort with glibc qsort. I will look into glibc implementation of qsort to get some insights.

[madebr]

When your compare function is not total I expect at least the following:

• it finishes,
• it does not expose undefined behavior
• it does not abort (abort != undefined behavior)
• it does not cause data loss (the array should still contain all elements)

[slouken]

When your compare function is not total I expect at least the following:

• it finishes,
• it does not expose undefined behavior
• it does not abort (abort != undefined behavior)
• it does not cause data loss (the array should still contain all elements)

I would not necessarily expect these things. We should compare them to glibc and msvc qsort functions to determine expected behavior.

[madebr]

I would not necessarily expect these things. We should compare them to glibc and msvc qsort functions to determine expected behavior.

posix says:

When the same objects (consisting of width bytes, irrespective of their current positions in the array) are passed more than once to the comparison function, the results shall be consistent with one another. That is, they shall define a total ordering on the array.

So random sort and non-total ordering are unspecified.

I tested with the libc of glibc and wine (with --slow-checks), and all tests are in green so my assumptions are valid against their implementations.