Date: Sun, 27 Jan 2019 06:07:47 -0800 From: Devin Teske <dteske@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: Devin Teske <dteske@FreeBSD.org>, Stefan Esser <se@freebsd.org>, Colin Percival <cperciva@tarsnap.com>, rgrimes@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r343480 - head/lib/libfigpar Message-ID: <D94AD0FE-164D-4D73-8813-53400C04644A@FreeBSD.org> In-Reply-To: <20190127154047.V900@besplex.bde.org> References: <201901262136.x0QLaAJv095518@pdx.rh.CN85.dnsmgr.net> <010001688c2cfc3e-e319d851-8b9e-4468-8bd1-f93331f35116-000000@email.amazonses.com> <a949233a-6689-369b-3a7f-2176fdc11d73@freebsd.org> <20190127154047.V900@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
> On Jan 26, 2019, at 9:31 PM, Bruce Evans <brde@optusnet.com.au> wrote: >=20 > On Sat, 26 Jan 2019, Stefan Esser wrote: >=20 >> Am 26.01.19 um 22:59 schrieb Colin Percival: >>> ... >>> The length of the string was already being recalculated, by strcpy. >>>=20 >>> It seems to me that this could be written as >>>=20 >>> temp =3D malloc(slen + 1); >>> if (temp =3D=3D NULL) /* could not allocate memory */ >>> return (-1); >>> memcpy(temp, source, slen + 1); >>>=20 >>> which avoids both recalculating the string length and using strcpy? >>=20 >> In principle you are right, there is a small run-time cost by not = using >> the known length for the allocation. >>=20 >> The Clang Scan checks are leading to lots (thousands) of false = positives >> and a I have looked at quite a number and abstained from silencing = them >> in the hope that the scan will be improved. >>=20 >> It should learn that at least the trivial case of an allocation of = the >> value of strlen()+1 followed by a strcpy (and no further strcat or = the >> like) is safe. >=20 > It would be a large bug in coverity for it complain about normal use = of > strcpy(). >=20 > However, the the use was not normal. It has very broken error = checking > for malloc(). This gave undefined behaviour for C99 and usually = failure > of the function and a memory leak for POSIX, >=20 > The broken error checking was to check errno instead of the return > value of malloc(), without even setting errno to 0 before calling > malloc(). This is especially broken in a library. It is normal for > errno to contain some garbage value from a previous failure, e.g., > ENOTTY from isatty(). So the expected behaviour of this library > function is to usually fail and leak the successfully malloc()ed = memory > when the broken code is reached. Coverity should find this bug. > Perhaps it did. >=20 > If errno were set before the call to malloc(), then the code would = just > be unportable. Setting errno when malloc() fails is a POSIX extension > of C99. Coverity should be aware of the standard being used, and = should > find this bug for C99 but not for POSIX. >=20 > The fix and the above patch don't fix styles bug related to the broken > check. First there is the lexical style bug of placing the comment > on the check instead of on the result of the check. The main bug is > that it should go without saying that malloc failures are checked for > by checking whether malloc() returned NULL. But in the old version, > the check was encrypted as the broken check of errno. The comment > decrypts this a little. >=20 I am genuinely flattered that so much thought is being placed around = code that I wrote circa 1999. My code there might even pre-date the C99 standard if memory serves. When I wrote that code, I had tested it on extensively on over 20 = different UNIX platforms. Compatibility was a nightmare back then. I'm so happy we have all these = wonderful shiny standards today. --=20 Cheers, Devin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D94AD0FE-164D-4D73-8813-53400C04644A>