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

Am 11.01.22 um 21:08 schrieb Mark Millard:
> On 2022-Jan-11, at 05:19, Stefan Esser <se@freebsd.org> wrote:
[...]
>> The undefined behavior is caused by insufficient checking of parameters
>> in mansearch.c.
>>
>> As part of the initializations performed at the start of mansearch(),
>> the variables cur and *res are initialized to 0 resp. NULL:
>>
>> 	cur = maxres = 0;	
>> 	if (res != NULL)
>> 		*res = NULL;
>>
>> If no match is found, these values are unchanged at line 223, where res
>> is checked to be non-NULL, but then *res is passed to qsort() and that
>> is still NULL.
>>
>> Suggested fix (also attached to avoid white-space issues):
>>
>> --- usr.bin/mandoc/mansearch.c
>> +++ usr.bin/mandoc/mansearch.c
>> @@ -220,7 +220,7 @@
>> 	if (cur && search->firstmatch)
>> 		break;
>> 	}
>> -	if (res != NULL)
>> +	if (res != NULL && *res != NULL)
>> 		qsort(*res, cur, sizeof(struct manpage), manpage_compare);
>> 	if (chdir_status && getcwd_status && chdir(buf) == -1)
>> 		warn("%s", buf);
>>
>> (File name as in OpenBSD, it is contrib/mandoc/mansearch.c in FreeBSD.)
> 
> Cool. Thanks.
> 
> (But I'm not a committer so someone else
> will have to deal with doing an update to
> the file in git --and likely MFC'ing it.)
> 
> ===
> Mark Millard
> marklmi at yahoo.com
> 

I have submitted a bug report to our upstream (OpenBSD), but the
issue could also be fixed (or rather undefined behavior prevented)
by a simple patch that makes qsort() detect the NULL pointer:

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.

I'll apply this patch tomorrow, if there are no objections.

Regards, STefan