Re: CPPCheck found 24 high risk bugs in Git v.1.8.3.4

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 19, 2013, 20:46 UTC
Message-ID: <7vmwodl885.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20130819204044.GA29258@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 62 quoted lines

> On Mon, Aug 19, 2013 at 09:03:21PM +0100, Philip Oakley wrote:
>
>> In case other readers don't have a .xlsx reader here is Rick's list
>> in plain text (may be white space damaged).
>> 
>> I expect some will be false positives, and some will just be being
>> too cautious.
>>
>> [...]
>> 
>> description resourceFilePath fileName lineNumber
>>      nullPointer(CppCheck) \git-master\builtin\add.c add.c 286
>
> Hm. That code in v1.8.3.4 reads:
>
>         if (pathspec)
>                 while (pathspec[pc])
>                         pc++;
>
> What's the problem? If pathspec is not properly terminated, we can run
> off the end, but I do see anything to indicate that is the case. What
> does the "nullPointer" check mean here?
>
>>      wrongPrintfScanfArgNum(CppCheck) \git-master\builtin\fetch.c
>> fetch.c 588
>
> Line 588 does not have formatted I/O at all. Are these line numbers
> somehow not matching what I have in v1.8.3.4?
>
>>      nullPointer(CppCheck) \git-master\builtin\ls-files.c ls-files.c
>> 144
>
> This one looks like:
>
>        if (tag && *tag && show_valid_bit &&
>             (ce->ce_flags & CE_VALID)) {
>                 static char alttag[4];
>                 memcpy(alttag, tag, 3);
>                 if (isalpha(tag[0]))
>
> where the final line is 144. But we have explicitly checked that tag is not
> NULL...
>
>>      doubleFree(CppCheck) \git-master\builtin\notes.c notes.c 275
>
> This one looks like:
>
>   if (...) {
>     free(buf);
>     die(...);
>   }
>   ...
>   free(buf);
>
> which might look like a double free if you do not know that die() will
> never return (it is properly annotated for gcc, but I don't know whether
> CppCheck understands such things).
>
> So out of the 4 entries I investigated, none of them looks like an
> actual problem. But I'm not even sure I am looking at the right place;
> these don't even seem like things that would cause a false positive in a
> static analyzer.

And the ones I picked at random looks totally bogus, too.

     uninitvar(CppCheck) \git-master\notes.c notes.c 805
     uninitvar(CppCheck) \git-master\notes.c notes.c 805

That is

        int combine_notes_concatenate(unsigned char *cur_sha1,
                        const unsigned char *new_sha1)
        {
                char *cur_msg = NULL, *new_msg = NULL, *buf;
                unsigned long cur_len, new_len, buf_len;
                enum object_type cur_type, new_type;
                int ret;

                /* read in both note blob objects */
                if (!is_null_sha1(new_sha1))
                        new_msg = read_sha1_file(new_sha1, &new_type, &new_len);
805:            if (!new_msg || !new_len || new_type != OBJ_BLOB) {
                        free(new_msg);
                        return 0;
                }

new_msg starts out to be NULL, if we did not run read_sha1_file(), it will still be NULL and "if()" will not look at new_len/new_type so their being uninitialized does not matter. If we did run read_sha1_file(), and if it returns a non-NULL new_msg, both of these are filled. If read_sha1_file() returns a NULL new_msg, again the other two does not matter.

In short, the analyzer seems to be giving useless noise for this one.