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

From: Jeff King <peff@peff.net>
Date: Aug 19, 2013, 20:40 UTC
Message-ID: <20130819204044.GA29258@sigill.intra.peff.net>
In-Reply-To: <33FCCE7A2CD5494B88F3C5CDBC341DFD@PhilipOakley>

On Mon, Aug 19, 2013 at 09:03:21PM +0100, Philip Oakley wrote:

Show 10 quoted lines

> 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.

-Peff