svn commit: r343480 - head/lib/libfigpar

[Devin Teske]

> On Jan 26, 2019, at 9:31 PM, Bruce Evans <brde at 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