From owner-svn-src-head@freebsd.org Sun Jan 27 14:08:51 2019 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id B653E14AD87E; Sun, 27 Jan 2019 14:08:51 +0000 (UTC) (envelope-from devin@shxd.cx) Received: from shxd.cx (mail.shxd.cx [64.201.244.140]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 512736B127; Sun, 27 Jan 2019 14:08:51 +0000 (UTC) (envelope-from devin@shxd.cx) Received: from [76.77.180.168] (port=54347 helo=eskarina.lan) by shxd.cx with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.77 (FreeBSD)) (envelope-from ) id 1gneMe-000N0i-SW; Sat, 26 Jan 2019 22:56:20 -0800 Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\)) Subject: Re: svn commit: r343480 - head/lib/libfigpar From: Devin Teske In-Reply-To: <20190127154047.V900@besplex.bde.org> Date: Sun, 27 Jan 2019 06:07:47 -0800 Cc: Devin Teske , Stefan Esser , Colin Percival , rgrimes@freebsd.org, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Transfer-Encoding: quoted-printable Message-Id: References: <201901262136.x0QLaAJv095518@pdx.rh.CN85.dnsmgr.net> <010001688c2cfc3e-e319d851-8b9e-4468-8bd1-f93331f35116-000000@email.amazonses.com> <20190127154047.V900@besplex.bde.org> To: Bruce Evans X-Mailer: Apple Mail (2.3445.9.1) Sender: devin@shxd.cx X-Rspamd-Queue-Id: 512736B127 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.93 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.93)[-0.926,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 27 Jan 2019 14:08:52 -0000 > On Jan 26, 2019, at 9:31 PM, Bruce Evans 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