From nobody Wed Jan 12 12:44:44 2022 X-Original-To: freebsd-current@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id D5F831941FAC for ; Wed, 12 Jan 2022 12:44:49 +0000 (UTC) (envelope-from se@FreeBSD.org) Received: from smtp.freebsd.org (smtp.freebsd.org [IPv6:2610:1c1:1:606c::24b:4]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "smtp.freebsd.org", Issuer "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4JYnMP20Dfz3tvd; Wed, 12 Jan 2022 12:44:49 +0000 (UTC) (envelope-from se@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1641991489; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=iAqFWz5ktVQQ2yY0/MEcBgP/KqBt2VokzNtoQNq07Hk=; b=nBUDoRk2Gtg8OL84rDp2IX+1BTOknbXQNSd7aFr1d8bKi+t8CS/tbhVDnMcKx87ClRSnDP crUVeZIsAzg22scsOYodch64RJtU2BRK/xv70NNp/G6QHAV2YOW852MDxLU9578WrcMGNJ w9fItbIqrVGeSU6c5Q52YdANHXVQCmRXlBaduvytEQpqa3u2IyB/gCCpJu5xOfUxGBwQSG dUJ32r0u6nLBe/YR1d8LjlIGuoAhGYRV6oIO5cEFQ9goscL8MpSShEN3EYbIrfedEvSqTQ LtjETh8OH0iBnlSqh6NLAkavzCdIsr1hEA3lDY5aWx7M2jjQ9mU00V45QSgzkA== Received: from [IPV6:2003:cd:5f26:900:d9d2:5b4c:aa73:bc5b] (p200300cd5f260900d9d25b4caa73bc5b.dip0.t-ipconnect.de [IPv6:2003:cd:5f26:900:d9d2:5b4c:aa73:bc5b]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client did not present a certificate) (Authenticated sender: se/mail) by smtp.freebsd.org (Postfix) with ESMTPSA id 0DEC5200DB; Wed, 12 Jan 2022 12:44:47 +0000 (UTC) (envelope-from se@FreeBSD.org) Message-ID: Date: Wed, 12 Jan 2022 13:44:44 +0100 List-Id: Discussions about the use of FreeBSD-current List-Archive: https://lists.freebsd.org/archives/freebsd-current List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-freebsd-current@freebsd.org MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Subject: Re: UBSAN report for main [so: 14] /usr/bin/whatis: non-zero (48) and zero offsets from null pointer in qsort.c Content-Language: en-US To: =?UTF-8?Q?Jan_Kokem=c3=bcller?= , Mark Millard Cc: bugs@openbsd.org, freebsd-current , Baptiste Daroussin References: <35333abc-9d4a-4b78-586d-1e869df4f9d4@FreeBSD.org> <7babd754-6dab-223a-7bfd-ff06f10c71e2@FreeBSD.org> <80e1f514-c0b3-cf79-ea6f-8c62cb1db386@gmail.com> From: Stefan Esser In-Reply-To: <80e1f514-c0b3-cf79-ea6f-8c62cb1db386@gmail.com> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="------------bRIpNhbcXHLAE6JIYyS9A76J" ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1641991489; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=iAqFWz5ktVQQ2yY0/MEcBgP/KqBt2VokzNtoQNq07Hk=; b=ukbhCXUwk9I/m4Lhr8KKGe7wn3DdYhtxKTGJ2l5nERa/YQhC99XoGVbNEyWdNsSyCY2EgZ 7CIqYVInZaDqHiGKR4n2z0d1TGYrr5Dsx0Y/5dqVu+w+bMrgOqN+zQX+v9eQnNK+itFKG4 qFTc5qqN0DpEMAJ0hKSCBl2XPaMGHVf1NVdBmSJRyohakEc/aCtmfKCvgCjys3J5KN6KDt E1pNvVGyw9V3+kAKAeAVA0/sLbIwhfnrUlTbBsisJHgZcJ7poM5eWbXXoeKVewHjuNkpiw 0i5zc/xKTe931RfnAWb0PatmDBOX/udrglrOUIhftY3kYggMGmhwe+rzdQZ5Og== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1641991489; a=rsa-sha256; cv=none; b=NLoKPBolOLSSFysHa7pUkpuVq9E1MU5/iP46SDRYYKsPw8iP/QO2oemIk98SsYA6sWlb92 ITO6BeIsnanBo2osm4aR2tRD5jJL3TGtHnMDrISW/i+FUJjnnzO5w8Q28lCmP5eS2uNXu3 83f+nzcxIbg3WiOj3sdCi+Bbf8DujUAu48ttmsTXKrvE0AmPMxjDMoWCwBoWkW21Stc2Wo 6NnPON9vyMPxHn6GoAfSW9IsveQGvuQmqbbCAogbY4Rsd3/VU5TSLIyms8jLInINHzJPPs jtK7jaViUKdFLwZL/LMIbaJUkvrNiRHxTuTak7OZgxD65I9KYwKTGpNSvq7JPw== ARC-Authentication-Results: i=1; mx1.freebsd.org; none X-ThisMailContainsUnwantedMimeParts: N This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --------------bRIpNhbcXHLAE6JIYyS9A76J Content-Type: multipart/mixed; boundary="------------sWzOcjTR2UPOZjVfWKWfqCl2"; protected-headers="v1" From: Stefan Esser To: =?UTF-8?Q?Jan_Kokem=c3=bcller?= , Mark Millard Cc: bugs@openbsd.org, freebsd-current , Baptiste Daroussin Message-ID: Subject: Re: UBSAN report for main [so: 14] /usr/bin/whatis: non-zero (48) and zero offsets from null pointer in qsort.c References: <35333abc-9d4a-4b78-586d-1e869df4f9d4@FreeBSD.org> <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--