Date: Sun, 27 Jan 2019 16:38:45 -0700 From: Warner Losh <imp@bsdimp.com> To: Devin Teske <dteske@freebsd.org> Cc: Bruce Evans <brde@optusnet.com.au>, Stefan Esser <se@freebsd.org>, Colin Percival <cperciva@tarsnap.com>, "Rodney W. Grimes" <rgrimes@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r343480 - head/lib/libfigpar Message-ID: <CANCZdfrsoYzuzd5pc5n6ps7ev1PHcZyTSZhJHxMvE5B-TdLBRA@mail.gmail.com> In-Reply-To: <D94AD0FE-164D-4D73-8813-53400C04644A@FreeBSD.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> <D94AD0FE-164D-4D73-8813-53400C04644A@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, Jan 27, 2019 at 7:09 AM Devin Teske <dteske@freebsd.org> wrote: > > > > 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. > Yea, Unix compatibility was great through '77 or so (since even though there was some minor diversity inside of Bell Labs, it wasn't so large and the extant code base was small enough to cope). The jump form V6 to V7 broke a lot of interfaces, so people started doing portability hacks. It got worse as BSD released its evolved versions, and USG started to go in a different direction with System III. Then the Unix Wars happened during the 80's. By 1999 these had mostly settled out and most of the portability mess as but a mere echo of what it had grown to in the early 80's (the curious can go look at the TTY interface diversity that Kermit pandered to until the late 90's when it grew too large to run on most of the crazy systems it had compat code for and started trimming the ancient ones... I think it supported 6 or 7 different variants of Unix that ran on the PDP-11 alone... though to be fair, the tty interface was/is the hardest to write portably, even today). Today, the fast moving target is all the new Linux system calls and weird library routines, but much of that makes it into standards bodies and FreeBSD keeps up enough to knock most of the rough edges off... Warner
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CANCZdfrsoYzuzd5pc5n6ps7ev1PHcZyTSZhJHxMvE5B-TdLBRA>