> On Jan 26, 2019, at 9:31 PM, Bruce Evans <b...@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
_______________________________________________
svn-src-head@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"