Re: UBSAN report for main [so: 14] /usr/bin/whatis: non-zero (48) and zero offsets from null pointer in qsort.c

On 2022-Jan-11, at 23:50, Jan Kokemüller <jan.kokemueller@gmail.com> wrote:

> On 11.01.22 22:08, Stefan Esser wrote:
>> diff --git a/lib/libc/stdlib/qsort.c b/lib/libc/stdlib/qsort.c
>> index 5016fff7895f..51c41e802330 100644
>> --- a/lib/libc/stdlib/qsort.c
>> +++ b/lib/libc/stdlib/qsort.c
>> @@ -108,6 +108,8 @@ local_qsort(void *a, size_t n, size_t es, cmp_t *cmp, void
>> *thunk)
>> 	int cmp_result;
>> 	int swap_cnt;
>> 
>> +	if (__predict_false(a == NULL))
>> +		return;
>> loop:
>> 	swap_cnt = 0;
>> 	if (n < 7) {
>> 
>> This would also work to prevent the NULL pointer arithmetik for
>> ports that might also path a == NULL and n == 0 in certain cases.
> 
> The UB happens in this line, when "a == NULL" and "n == 0", right?
> 
>    for (pm = (char *)a + es; pm < (char *)a + n * es; pm += es)
> 
> This is arithmetic on a pointer (the NULL pointer) which is not part of an
> array, which is UB.
> 
> Then, wouldn't "if (__predict_false(n == 0))" be more appropriate than checking
> for "a == NULL" here? Testing for "a == NULL" might suppress UBSAN warnings of
> valid bugs, i.e. when "qsort" is called with "a == NULL" and "n != 0". In that
> case UBSAN _should_ trigger.
> 
> UBSAN should not trigger when n == 0, though. At least, when "a" does point to
> a valid array. But what about the case of "a == NULL && n == 0"? Is that deemed
> UB? It looks like at least FreeBSD's "qsort_s" implementation says it's legal.
> 
> a != NULL (pointing to valid array), n != 0  ->  "normal" case, no UB
> a != NULL (pointing to valid array), n == 0  ->  should not trigger UB, and
>                                                 doesn't in the current
>                                                 implementation
> a == NULL, n == 0                            ->  should not trigger UB?
>                                                 (debatable)
> 
> So if "a == NULL && n == 0" was deemed legal, then there would be no bug in
> "mansearch.c", right?
> 

ISO/IEC 9899:2011 (E) is not explicit about such things for
qsort, nor is POSIX as I remember: POSIX states that in cases
of disagreement it defers to a C standard, if I remember right.

But ISO/IEC 9899:2011 (E) is somewhat explicit for qsort_s:
(parameters: base, nmemb, size, and compar in that order)

QUOTE
If nmemb is not equal to zero,  then nether base nor compar
shall be a null pointer.
END QUOTE

But there are no words about nmemb==0 relative to either of:

base vs. NULL
compar vs. NULL

So far as I can tell, the implementation is free to treat
nmemb==0 && (base==NULL||compar==NULL) as a "runtime-constraint
violation" for qsort_s and to return a non-zero value --or to
not do so and return zero.

As qsort does not return a value, any rejection of such a
combination for qsort would be in a more drastic form, making
such an unlikely choice. (qsort is not documented to assign
errno either.)

So I would expect qsort to avoid involving undefined behavior
when nmemb==0 && (base==NULL||compar==NULL) but to not reject
the condition. I do not take doing a well-defined "no-op" as
a rejection for my wording here.


===
Mark Millard
marklmi at yahoo.com