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: > > On Sat, 26 Jan 2019, Stefan Esser wrote: > >> Am 26.01.19 um 22:59 schrieb Colin Percival: >>> ... >>> The length of the string was already being recalculated, by strcpy. >>> >>> It seems to me that this could be written as >>> >>> temp = malloc(slen + 1); >>> if (temp == NULL) /* could not allocate memory */ >>> return (-1); >>> memcpy(temp, source, slen + 1); >>> >>> which avoids both recalculating the string length and using strcpy? >> >> In principle you are right, there is a small run-time cost by not using >> the known length for the allocation. >> >> 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. >> >> 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. > > It would be a large bug in coverity for it complain about normal use of > strcpy(). > > 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, > > 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. > > 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. > > 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. > 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. -- Cheers, Devin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?D94AD0FE-164D-4D73-8813-53400C04644A>