Date: Wed, 12 Jan 2022 13:44:44 +0100 From: Stefan Esser <se@FreeBSD.org> To: =?UTF-8?Q?Jan_Kokem=c3=bcller?= <jan.kokemueller@gmail.com>, Mark Millard <marklmi@yahoo.com> Cc: bugs@openbsd.org, freebsd-current <freebsd-current@freebsd.org>, Baptiste Daroussin <bapt@FreeBSD.org> Subject: Re: UBSAN report for main [so: 14] /usr/bin/whatis: non-zero (48) and zero offsets from null pointer in qsort.c Message-ID: <b38c57e4-554d-dd46-dbfd-2ea961fe45b4@FreeBSD.org> In-Reply-To: <80e1f514-c0b3-cf79-ea6f-8c62cb1db386@gmail.com> References: <A4577E70-AB32-450F-A3F6-A2B42B09A1B3.ref@yahoo.com> <A4577E70-AB32-450F-A3F6-A2B42B09A1B3@yahoo.com> <35333abc-9d4a-4b78-586d-1e869df4f9d4@FreeBSD.org> <BEFB4665-F32B-4AA0-BE4A-5ABB8B973012@yahoo.com> <7babd754-6dab-223a-7bfd-ff06f10c71e2@FreeBSD.org> <80e1f514-c0b3-cf79-ea6f-8c62cb1db386@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
This is an OpenPGP/MIME signed message (RFC 4880 and 3156)
--------------bRIpNhbcXHLAE6JIYyS9A76J
Content-Type: multipart/mixed; boundary="------------sWzOcjTR2UPOZjVfWKWfqCl2";
protected-headers="v1"
From: Stefan Esser <se@FreeBSD.org>
To: =?UTF-8?Q?Jan_Kokem=c3=bcller?= <jan.kokemueller@gmail.com>,
Mark Millard <marklmi@yahoo.com>
Cc: bugs@openbsd.org, freebsd-current <freebsd-current@freebsd.org>,
Baptiste Daroussin <bapt@FreeBSD.org>
Message-ID: <b38c57e4-554d-dd46-dbfd-2ea961fe45b4@FreeBSD.org>
Subject: Re: UBSAN report for main [so: 14] /usr/bin/whatis: non-zero (48) and
zero offsets from null pointer in qsort.c
References: <A4577E70-AB32-450F-A3F6-A2B42B09A1B3.ref@yahoo.com>
<A4577E70-AB32-450F-A3F6-A2B42B09A1B3@yahoo.com>
<35333abc-9d4a-4b78-586d-1e869df4f9d4@FreeBSD.org>
<BEFB4665-F32B-4AA0-BE4A-5ABB8B973012@yahoo.com>
<7babd754-6dab-223a-7bfd-ff06f10c71e2@FreeBSD.org>
<80e1f514-c0b3-cf79-ea6f-8c62cb1db386@gmail.com>
In-Reply-To: <80e1f514-c0b3-cf79-ea6f-8c62cb1db386@gmail.com>
--------------sWzOcjTR2UPOZjVfWKWfqCl2
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Am 12.01.22 um 08:50 schrieb Jan Kokem=C3=BCller:
> 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 *c=
mp, void
>> *thunk)
>> int cmp_result;
>> int swap_cnt;
>>
>> + if (__predict_false(a =3D=3D NULL))
>> + return;
>> loop:
>> swap_cnt =3D 0;
>> if (n < 7) {
>>
>> This would also work to prevent the NULL pointer arithmetik for
>> ports that might also path a =3D=3D NULL and n =3D=3D 0 in certain cas=
es.
>=20
> The UB happens in this line, when "a =3D=3D NULL" and "n =3D=3D 0", rig=
ht?
>=20
> for (pm =3D (char *)a + es; pm < (char *)a + n * es; pm +=3D es)
>=20
> This is arithmetic on a pointer (the NULL pointer) which is not part of=
an
> array, which is UB.
Yes.
> Then, wouldn't "if (__predict_false(n =3D=3D 0))" be more appropriate t=
han checking
> for "a =3D=3D NULL" here? Testing for "a =3D=3D NULL" might suppress UB=
SAN warnings of
> valid bugs, i.e. when "qsort" is called with "a =3D=3D NULL" and "n !=3D=
0". In that
> case UBSAN _should_ trigger.
Yes, but not only UBSAN would trigger, the program would probably
crash due to an attempt to access an unmapped page.
> UBSAN should not trigger when n =3D=3D 0, though. At least, when "a" do=
es point to
> a valid array. But what about the case of "a =3D=3D NULL && n =3D=3D 0"=
? Is that deemed
> UB? It looks like at least FreeBSD's "qsort_s" implementation says it's=
legal.
This might be legal, but it leads to adding the element size to a
NULL pointer, in the current implementation. The addition happens
in the initialization part of the for loop, before n =3D=3D 0 leads to
no actual iteration being performed (a + es < a + n * es is false
for es > 0).
There is no functional difference if the case of a =3D=3D NULL and
n =3D=3D 0 is silently ignored.
But your are correct: just returning early for a =3D=3D NULL and n !=3D 0=
will prevent the program abort.
> a !=3D NULL (pointing to valid array), n !=3D 0 -> "normal" case, no =
UB
> a !=3D NULL (pointing to valid array), n =3D=3D 0 -> should not trigg=
er UB, and
> doesn't in the current=
> implementation
It does trigger UB in a way that does not cause issues (or else
the problem would have been detected before). a =3D=3D NULL makes the
calculation of pm =3D (char *)a + es undefined, but the value of pm
will never be used if n =3D=3D 0.
> a =3D=3D NULL, n =3D=3D 0 -> should not tri=
gger UB?
> (debatable)
>=20
> So if "a =3D=3D NULL && n =3D=3D 0" was deemed legal, then there would =
be no bug in
> "mansearch.c", right?
IMHO it is not the question of "legal" or not, but we should prevent
the undefined behavior that results from execution reaching the
initialization part of the for loop.
Any you are correct, the patch should probably be:
diff --git a/lib/libc/stdlib/qsort.c b/lib/libc/stdlib/qsort.c
index 5016fff7895f..eef51d2dd3b3 100644
--- a/lib/libc/stdlib/qsort.c
+++ b/lib/libc/stdlib/qsort.c
@@ -108,6 +108,8 @@
int cmp_result;
int swap_cnt;
+ if (__predict_false(a =3D=3D NULL && n =3D=3D 0))
+ return;
loop:
swap_cnt =3D 0;
if (n < 7) {
This will be detected by UBSAN if called with a =3D=3D NULL and n !=3D 0,=
but it will also cause the program to fail with typical parameters
for the elements to sort and the cmp function.
Regards, STefan
PS: I just saw Mark's reply regarding n =3D=3D 0 and cmp =3D=3D NULL. Tha=
t
case is already covered, since n =3D=3D 0 will prevent cmp from
being dereferenced (since that only happens in the loop body,
which will not be entered for n =3D=3D 0).
--------------sWzOcjTR2UPOZjVfWKWfqCl2--
--------------bRIpNhbcXHLAE6JIYyS9A76J
Content-Type: application/pgp-signature; name="OpenPGP_signature.asc"
Content-Description: OpenPGP digital signature
Content-Disposition: attachment; filename="OpenPGP_signature"
-----BEGIN PGP SIGNATURE-----
wsB5BAABCAAjFiEEo3HqZZwL7MgrcVMTR+u171r99UQFAmHezTwFAwAAAAAACgkQR+u171r99USs
0ggAjg4TpVg6CHmFOCQpOHEtDapGG74qMzORO3AEBbCl5oZfXeYwds8c7AnOGbF2pjwGYg1xqFFU
sLXZTzUG5E/MtJCPF9x5KqovxwloqeCBwfJqafRRhlWBx4qiKBC3eVAr322zeWJKHDU7oIc5kNQk
ocPN5yAtSPu59YB5vxWUuVoM5wyOCwjCIB9HfMV05ziuQF2Z6ih6KBtVfAcgDEMXsk/hIYUyQFMM
oOe1ksqvx1MSwrHiU6NH9i7YSjQbgh80K2Dt25Mu5yVkU5yf7nVvxntJsG5vxOFaeM4NVNLKOeMN
gwTiVZymYs4jue0HsjnK/FaEJLlye+y8RP+k2KP8sw==
=WBax
-----END PGP SIGNATURE-----
--------------bRIpNhbcXHLAE6JIYyS9A76J--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?b38c57e4-554d-dd46-dbfd-2ea961fe45b4>