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

Jeff King [off-list ref] writes:

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

quoted

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?

quoted

     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?

quoted

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

quoted

     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.

On Mon, Aug 19, 2013 at 10:46 PM, Junio C Hamano [off-list ref] wrote:

Jeff King [off-list ref] writes:

quoted

On Mon, Aug 19, 2013 at 09:03:21PM +0100, Philip Oakley wrote:
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

FWIW, I looked at the 3 notes-related ones and reached the same conclusions.

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net