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

[Jeff King <peff@peff.net>]

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.

-Peff