From owner-svn-src-all@freebsd.org Sun Jan 27 05:31:51 2019 Return-Path: Delivered-To: svn-src-all@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 D7DF714BFCFE; Sun, 27 Jan 2019 05:31:50 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from mail105.syd.optusnet.com.au (mail105.syd.optusnet.com.au [211.29.132.249]) by mx1.freebsd.org (Postfix) with ESMTP id 5329E87B9E; Sun, 27 Jan 2019 05:31:49 +0000 (UTC) (envelope-from brde@optusnet.com.au) Received: from [192.168.0.102] (c110-21-101-228.carlnfd1.nsw.optusnet.com.au [110.21.101.228]) by mail105.syd.optusnet.com.au (Postfix) with ESMTPS id 3FBEB10554EF; Sun, 27 Jan 2019 16:31:39 +1100 (AEDT) Date: Sun, 27 Jan 2019 16:31:34 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Stefan Esser cc: Colin Percival , 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 In-Reply-To: Message-ID: <20190127154047.V900@besplex.bde.org> References: <201901262136.x0QLaAJv095518@pdx.rh.CN85.dnsmgr.net> <010001688c2cfc3e-e319d851-8b9e-4468-8bd1-f93331f35116-000000@email.amazonses.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.2 cv=FNpr/6gs c=1 sm=1 tr=0 a=PalzARQSbocsUSjMRkwAPg==:117 a=PalzARQSbocsUSjMRkwAPg==:17 a=kj9zAlcOel0A:10 a=Ic4zIQ0n0UbnqT2PbJQA:9 a=kmLDeWUIewAst9HY:21 a=ivDYyo8wYh5hi8Zk:21 a=CjuIK1q_8ugA:10 X-Rspamd-Queue-Id: 5329E87B9E X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.98 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.98)[-0.983,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 27 Jan 2019 05:31:51 -0000 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. Bruce