svn commit: r343480 - head/lib/libfigpar

[Bruce Evans]

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