CPPCheck found 24 high risk bugs in Git v.1.8.3.4

CPPCheck found 24 high risk bugs in Git v.1.8.3.4

[Koch, Rick (Subcontractor)]

[-- Attachment #1: Type: text/plain, Size: 423 bytes --]

I'm directing to this e-mail, as it seems to be the approved forum for posting Git bugs. We ran CPPCheck against Git v.1.8.3.4 and found 24 high risk bugs.  Please see the attachment xlsx. 

Is there a method to post to the Git community to allow the community to review and debunk as faults positive or develop patches to fix lists code files?

v/r

Roderick (Rick) Koch
Information Assurance
Rick.Koch@tbe.com


[-- Attachment #2: CPPCheck_on_Git.v1.8.3.4.xlsx --]
[-- Type: application/vnd.openxmlformats-officedocument.spreadsheetml.sheet, Size: 10741 bytes --]

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

[Philip Oakley]

From: "Koch, Rick (Subcontractor)" <Rick.Koch@tbe.com>
Sent: Monday, August 19, 2013 6:09 PM
>I'm directing to this e-mail, as it seems to be the approved forum
>for posting Git bugs. We ran CPPCheck against Git v.1.8.3.4
>and found 24 high risk bugs. Please see the attachment xlsx.

>Is there a method to post to the Git community to allow the
>community to review and debunk as faults positive or develop
>patches to fix lists code files?

>v/r

>Roderick (Rick) Koch
>Information Assurance
>Rick.Koch@tbe.com

What OS version / CPPCheck version was this checked on?

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.

Philip

 description resourceFilePath fileName lineNumber
      nullPointer(CppCheck) \git-master\builtin\add.c add.c 286
      wrongPrintfScanfArgNum(CppCheck) \git-master\builtin\fetch.c
fetch.c 588
      nullPointer(CppCheck) \git-master\builtin\ls-files.c ls-files.c
144
      nullPointer(CppCheck) \git-master\builtin\merge.c merge.c 1208
      doubleFree(CppCheck) \git-master\builtin\notes.c notes.c 275
      nullPointer(CppCheck) \git-master\builtin\reflog.c reflog.c 437
      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
2803
      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
2802
      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
2805
      memleakOnRealloc(CppCheck) \git-master\compat\win32\syslog.c
syslog.c 46
      uninitvar(CppCheck)
\git-master\contrib\examples\builtin-fetch--tool.c builtin-fetch--tool.c
419
      uninitvar(CppCheck) \git-master\fast-import.c fast-import.c 2917
      nullPointer(CppCheck) \git-master\line-log.c line-log.c 638
      nullPointer(CppCheck) \git-master\mailmap.c mailmap.c 156
      uninitvar(CppCheck) \git-master\merge-recursive.c
merge-recursive.c 1887
      uninitvar(CppCheck) \git-master\notes.c notes.c 805
      uninitvar(CppCheck) \git-master\notes.c notes.c 805
      deallocret(CppCheck) \git-master\pretty.c pretty.c 677
      resourceLeak(CppCheck) \git-master\refs.c refs.c 3041
      doubleFree(CppCheck) \git-master\sequencer.c sequencer.c 924
      nullPointer(CppCheck) \git-master\sha1_file.c sha1_file.c 125
      doubleFree(CppCheck) \git-master\shell.c shell.c 130

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Johan Herland]

On Mon, Aug 19, 2013 at 10:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>> 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, <johan@herland.net>
www.herland.net

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

[Stefan Beller]

[-- Attachment #1: Type: text/plain, Size: 1505 bytes --]

On 08/19/2013 07:09 PM, Koch, Rick (Subcontractor) wrote:
> I'm directing to this e-mail, as it seems to be the approved forum for posting Git bugs. We ran CPPCheck against Git v.1.8.3.4 and found 24 high risk bugs.  Please see the attachment xlsx. 
> 
> Is there a method to post to the Git community to allow the community to review and debunk as faults positive or develop patches to fix lists code files?
> 

Hi,

if you're using cppcheck as found at https://github.com/danmar/cppcheck 
or http://sourceforge.net/apps/trac/cppcheck/ you really need to review 
the results, as there are many false positives.

I used that tool for my contributions so far (bug fixes as reported by cppcheck).
However you *really* need to manually review any message cppcheck generates.
This is because git is using a C, asm-like coding style for many routines,
whereas that cppcheck is rather optimized to find typical C++ errors.
And the styles vary wildy! (cppcheck tries to become no false positives, 
but it's hard I guess)

I am running that cppcheck tool on git regulary (cppcheck master branch on
git master branch), and review for real findings, you're welcome to do so
as well. :) 

There are other static code analyzers, which have slightly different 
goals, such as http://css.csail.mit.edu/stack/ which has an incredibly 
low false positive rate (I found none as of now).
However I think having different tools is a great thing, but you'd need
to know your tools. ;)

Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 899 bytes --]

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

[Philip Oakley]

From: "Koch, Rick (Subcontractor)" <Rick.Koch@tbe.com>

> Ran CPPCheck 1.5.6 on Windows-XP.

Hi Rick,
Thank you for the clarification.
Normal practice on the list is to use Reply All, so everyone can 
participate in the discussion.

It looks like most of the reports are false positives. My bikeshedding 
thought would be that it is common in Git to inspect all the call sites 
such that they don't create the various problems, rather than protect 
against the problems within the various functions, which may be a cause 
of the reports (i.e. different philosophical approach to checking).

regards

Philip
---

v/r

Roderick (Rick) Koch
OSF - Information Assurance
Team Teledyne / Sentar Inc.
Work: 256-726-1253
Rick.Koch@tbe.com


-----Original Message-----
From: Philip Oakley [mailto:philipoakley@iee.org]
Sent: Monday, August 19, 2013 3:03 PM
To: Koch, Rick (Subcontractor); Git List
Subject: Re: CPPCheck found 24 high risk bugs in Git v.1.8.3.4

From: "Koch, Rick (Subcontractor)" <Rick.Koch@tbe.com>
Sent: Monday, August 19, 2013 6:09 PM
>I'm directing to this e-mail, as it seems to be the approved forum for
>posting Git bugs. We ran CPPCheck against Git v.1.8.3.4 and found 24
>high risk bugs. Please see the attachment xlsx.

>Is there a method to post to the Git community to allow the community
>to review and debunk as faults positive or develop patches to fix lists
>code files?

>v/r

>Roderick (Rick) Koch
>Information Assurance
>Rick.Koch@tbe.com

What OS version / CPPCheck version was this checked on?

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.

Philip

 description resourceFilePath fileName lineNumber
      nullPointer(CppCheck) \git-master\builtin\add.c add.c 286
      wrongPrintfScanfArgNum(CppCheck) \git-master\builtin\fetch.c 
fetch.c 588
      nullPointer(CppCheck) \git-master\builtin\ls-files.c ls-files.c
144
      nullPointer(CppCheck) \git-master\builtin\merge.c merge.c 1208
      doubleFree(CppCheck) \git-master\builtin\notes.c notes.c 275
      nullPointer(CppCheck) \git-master\builtin\reflog.c reflog.c 437
      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
2803
      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
2802
      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
2805
      memleakOnRealloc(CppCheck) \git-master\compat\win32\syslog.c 
syslog.c 46
      uninitvar(CppCheck)
\git-master\contrib\examples\builtin-fetch--tool.c builtin-fetch--tool.c
419
      uninitvar(CppCheck) \git-master\fast-import.c fast-import.c 2917
      nullPointer(CppCheck) \git-master\line-log.c line-log.c 638
      nullPointer(CppCheck) \git-master\mailmap.c mailmap.c 156
      uninitvar(CppCheck) \git-master\merge-recursive.c 
merge-recursive.c 1887
      uninitvar(CppCheck) \git-master\notes.c notes.c 805
      uninitvar(CppCheck) \git-master\notes.c notes.c 805
      deallocret(CppCheck) \git-master\pretty.c pretty.c 677
      resourceLeak(CppCheck) \git-master\refs.c refs.c 3041
      doubleFree(CppCheck) \git-master\sequencer.c sequencer.c 924
      nullPointer(CppCheck) \git-master\sha1_file.c sha1_file.c 125
      doubleFree(CppCheck) \git-master\shell.c shell.c 130

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

[Philip Oakley]

----- Original Message ----- 
From: "Philip Oakley" <philipoakley@iee.org>
> From: "Koch, Rick (Subcontractor)" <Rick.Koch@tbe.com>
> Sent: Monday, August 19, 2013 6:09 PM
>>I'm directing to this e-mail, as it seems to be the approved forum
>>for posting Git bugs. We ran CPPCheck against Git v.1.8.3.4
>>and found 24 high risk bugs. Please see the attachment xlsx.
>
>>Is there a method to post to the Git community to allow the
>>community to review and debunk as faults positive or develop
>>patches to fix lists code files?
>
>>v/r
>
>>Roderick (Rick) Koch
>>Information Assurance
>>Rick.Koch@tbe.com
>
> What OS version / CPPCheck version was this checked on?
>
> 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.
>
> Philip
>
> description resourceFilePath fileName lineNumber
>      nullPointer(CppCheck) \git-master\builtin\add.c add.c 286
>      wrongPrintfScanfArgNum(CppCheck) \git-master\builtin\fetch.c
> fetch.c 588
>      nullPointer(CppCheck) \git-master\builtin\ls-files.c ls-files.c
> 144
>      nullPointer(CppCheck) \git-master\builtin\merge.c merge.c 1208
>      doubleFree(CppCheck) \git-master\builtin\notes.c notes.c 275
>      nullPointer(CppCheck) \git-master\builtin\reflog.c reflog.c 437
>      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
>      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
> 2803
>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
> 2802
>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
> 2805
>      memleakOnRealloc(CppCheck) \git-master\compat\win32\syslog.c
> syslog.c 46

This looks like a possible, based on 
http://bytes.com/topic/c/answers/215084-can-realloc-potentially-cause-memory-leak 
(Mac's reply, with tweaks)

"Misuse of realloc CAN cause a memory leak, but only when allocation 
fails"
"if realloc fails, the memory previously pointed to by 'str = 
realloc(str, ++str_len + 1)' will still be claimed, but you will have 
lost your only pointer to it, because realloc returns NULL on failure. 
This is a memory leak."

We (those using the compat function) then only provide a warning, so it 
could repeat endlessly.

Eric (cc'd) may be able to clarify if this is a possibility.

>      uninitvar(CppCheck)
> \git-master\contrib\examples\builtin-fetch--tool.c 
> builtin-fetch--tool.c
> 419
>      uninitvar(CppCheck) \git-master\fast-import.c fast-import.c 2917
>      nullPointer(CppCheck) \git-master\line-log.c line-log.c 638
>      nullPointer(CppCheck) \git-master\mailmap.c mailmap.c 156
>      uninitvar(CppCheck) \git-master\merge-recursive.c
> merge-recursive.c 1887
>      uninitvar(CppCheck) \git-master\notes.c notes.c 805
>      uninitvar(CppCheck) \git-master\notes.c notes.c 805
>      deallocret(CppCheck) \git-master\pretty.c pretty.c 677
>      resourceLeak(CppCheck) \git-master\refs.c refs.c 3041
>      doubleFree(CppCheck) \git-master\sequencer.c sequencer.c 924
>      nullPointer(CppCheck) \git-master\sha1_file.c sha1_file.c 125
>      doubleFree(CppCheck) \git-master\shell.c shell.c 130
>
> --
Philip 

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

[Erik Faye-Lund]

This one seems real, although it's quite theoretical. It should only happen
in cases where the log-message contains "%1", the initial malloc passed and
reallocing two more bytes failed.

However, what's much more of a disaster: "pos" is used after the call to
realloc might have moved the memory!

I guess something like this should fix both issues. Sorry about the
lack of indentation, it seems Gmail has regressed, and the old compose
mode is somehow gone... (also sorry for triple-posting to some of you,
Gmail seems particularly broken today)

diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index d015e43..0641f4e 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
  va_end(ap);

  while ((pos = strstr(str, "%1")) != NULL) {
- str = realloc(str, ++str_len + 1);
- if (!str) {
+ char *tmp = realloc(str, ++str_len + 1);
+ if (!tmp) {
  warning("realloc failed: '%s'", strerror(errno));
+ free(str);
  return;
  }
+ pos = tmp + (pos - str);
+ str = tmp;
  memmove(pos + 2, pos + 1, strlen(pos));
  pos[1] = ' ';
  }



On Tue, Aug 20, 2013 at 12:55 AM, Philip Oakley <philipoakley@iee.org> wrote:
> ----- Original Message ----- From: "Philip Oakley" <philipoakley@iee.org>
>
>> From: "Koch, Rick (Subcontractor)" <Rick.Koch@tbe.com>
>> Sent: Monday, August 19, 2013 6:09 PM
>>>
>>> I'm directing to this e-mail, as it seems to be the approved forum
>>> for posting Git bugs. We ran CPPCheck against Git v.1.8.3.4
>>> and found 24 high risk bugs. Please see the attachment xlsx.
>>
>>
>>> Is there a method to post to the Git community to allow the
>>> community to review and debunk as faults positive or develop
>>> patches to fix lists code files?
>>
>>
>>> v/r
>>
>>
>>> Roderick (Rick) Koch
>>> Information Assurance
>>> Rick.Koch@tbe.com
>>
>>
>> What OS version / CPPCheck version was this checked on?
>>
>> 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.
>>
>> Philip
>>
>> description resourceFilePath fileName lineNumber
>>      nullPointer(CppCheck) \git-master\builtin\add.c add.c 286
>>      wrongPrintfScanfArgNum(CppCheck) \git-master\builtin\fetch.c
>> fetch.c 588
>>      nullPointer(CppCheck) \git-master\builtin\ls-files.c ls-files.c
>> 144
>>      nullPointer(CppCheck) \git-master\builtin\merge.c merge.c 1208
>>      doubleFree(CppCheck) \git-master\builtin\notes.c notes.c 275
>>      nullPointer(CppCheck) \git-master\builtin\reflog.c reflog.c 437
>>      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
>>      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
>>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
>> 2803
>>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
>> 2802
>>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
>> 2805
>>      memleakOnRealloc(CppCheck) \git-master\compat\win32\syslog.c
>> syslog.c 46
>
>
> This looks like a possible, based on
> http://bytes.com/topic/c/answers/215084-can-realloc-potentially-cause-memory-leak
> (Mac's reply, with tweaks)
>
> "Misuse of realloc CAN cause a memory leak, but only when allocation fails"
> "if realloc fails, the memory previously pointed to by 'str = realloc(str,
> ++str_len + 1)' will still be claimed, but you will have lost your only
> pointer to it, because realloc returns NULL on failure. This is a memory
> leak."
>
> We (those using the compat function) then only provide a warning, so it
> could repeat endlessly.
>
> Eric (cc'd) may be able to clarify if this is a possibility.
>
>
>>      uninitvar(CppCheck)
>> \git-master\contrib\examples\builtin-fetch--tool.c builtin-fetch--tool.c
>> 419
>>      uninitvar(CppCheck) \git-master\fast-import.c fast-import.c 2917
>>      nullPointer(CppCheck) \git-master\line-log.c line-log.c 638
>>      nullPointer(CppCheck) \git-master\mailmap.c mailmap.c 156
>>      uninitvar(CppCheck) \git-master\merge-recursive.c
>> merge-recursive.c 1887
>>      uninitvar(CppCheck) \git-master\notes.c notes.c 805
>>      uninitvar(CppCheck) \git-master\notes.c notes.c 805
>>      deallocret(CppCheck) \git-master\pretty.c pretty.c 677
>>      resourceLeak(CppCheck) \git-master\refs.c refs.c 3041
>>      doubleFree(CppCheck) \git-master\sequencer.c sequencer.c 924
>>      nullPointer(CppCheck) \git-master\sha1_file.c sha1_file.c 125
>>      doubleFree(CppCheck) \git-master\shell.c shell.c 130
>>
>> --
>
> Philip

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

[Jeff King]

On Tue, Aug 20, 2013 at 01:15:02AM +0200, Erik Faye-Lund wrote:

> This one seems real, although it's quite theoretical. It should only happen
> in cases where the log-message contains "%1", the initial malloc passed and
> reallocing two more bytes failed.
> 
> However, what's much more of a disaster: "pos" is used after the call to
> realloc might have moved the memory!

Yeah, agreed on both counts.

> I guess something like this should fix both issues. Sorry about the
> lack of indentation, it seems Gmail has regressed, and the old compose
> mode is somehow gone... (also sorry for triple-posting to some of you,
> Gmail seems particularly broken today)
> 
> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index d015e43..0641f4e 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
>   va_end(ap);
> 
>   while ((pos = strstr(str, "%1")) != NULL) {
> - str = realloc(str, ++str_len + 1);
> - if (!str) {
> + char *tmp = realloc(str, ++str_len + 1);
> + if (!tmp) {
>   warning("realloc failed: '%s'", strerror(errno));
> + free(str);
>   return;
>   }
> + pos = tmp + (pos - str);
> + str = tmp;
>   memmove(pos + 2, pos + 1, strlen(pos));
>   pos[1] = ' ';
>   }

Yes, that looks like the right solution. You could also convert "pos" to
an integer index rather than a pointer (but then you end up adding it it
to the pointer in the memmove call, which is probably just as ugly).

-Peff

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

[Andreas Schwab]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
> index d015e43..0641f4e 100644
> --- a/compat/win32/syslog.c
> +++ b/compat/win32/syslog.c
> @@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
>   va_end(ap);
>
>   while ((pos = strstr(str, "%1")) != NULL) {
> - str = realloc(str, ++str_len + 1);
> - if (!str) {
> + char *tmp = realloc(str, ++str_len + 1);
> + if (!tmp) {
>   warning("realloc failed: '%s'", strerror(errno));
> + free(str);
>   return;
>   }
> + pos = tmp + (pos - str);

Pedantically, this is undefined (uses of both pos and str may trap after
realloc has freed the original pointer), it is better to calculate the
difference before calling realloc.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[René Scharfe]

Am 20.08.2013 20:44, schrieb Andreas Schwab:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
>> index d015e43..0641f4e 100644
>> --- a/compat/win32/syslog.c
>> +++ b/compat/win32/syslog.c
>> @@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
>>    va_end(ap);
>>
>>    while ((pos = strstr(str, "%1")) != NULL) {
>> - str = realloc(str, ++str_len + 1);
>> - if (!str) {
>> + char *tmp = realloc(str, ++str_len + 1);
>> + if (!tmp) {
>>    warning("realloc failed: '%s'", strerror(errno));
>> + free(str);
>>    return;
>>    }
>> + pos = tmp + (pos - str);
>
> Pedantically, this is undefined (uses of both pos and str may trap after
> realloc has freed the original pointer), it is better to calculate the
> difference before calling realloc.

And while at it, perhaps it's better to follow the suggestion in 
http://msdn.microsoft.com/en-us/library/aa363679.aspx under Remarks and 
replace "%1" with "%1!S!" instead of "% 1".

René

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

[Erik Faye-Lund]

On Tue, Aug 20, 2013 at 8:44 PM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
>> index d015e43..0641f4e 100644
>> --- a/compat/win32/syslog.c
>> +++ b/compat/win32/syslog.c
>> @@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
>>   va_end(ap);
>>
>>   while ((pos = strstr(str, "%1")) != NULL) {
>> - str = realloc(str, ++str_len + 1);
>> - if (!str) {
>> + char *tmp = realloc(str, ++str_len + 1);
>> + if (!tmp) {
>>   warning("realloc failed: '%s'", strerror(errno));
>> + free(str);
>>   return;
>>   }
>> + pos = tmp + (pos - str);
>
> Pedantically, this is undefined (uses of both pos and str may trap after
> realloc has freed the original pointer), it is better to calculate the
> difference before calling realloc.

I don't see how it's undefined. It's using the memory that 'pos'
*points to* that is undefined, no? The difference between 'pos' and
'str' should still be the same, it's not like realloc somehow
magically updates 'pos'...

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

[Erik Faye-Lund]

On Tue, Aug 20, 2013 at 10:34 PM, René Scharfe <l.s.r@web.de> wrote:
> Am 20.08.2013 20:44, schrieb Andreas Schwab:
>
>> Erik Faye-Lund <kusmabite@gmail.com> writes:
>>
>>> diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
>>> index d015e43..0641f4e 100644
>>> --- a/compat/win32/syslog.c
>>> +++ b/compat/win32/syslog.c
>>> @@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
>>>    va_end(ap);
>>>
>>>    while ((pos = strstr(str, "%1")) != NULL) {
>>> - str = realloc(str, ++str_len + 1);
>>> - if (!str) {
>>> + char *tmp = realloc(str, ++str_len + 1);
>>> + if (!tmp) {
>>>    warning("realloc failed: '%s'", strerror(errno));
>>> + free(str);
>>>    return;
>>>    }
>>> + pos = tmp + (pos - str);
>>
>>
>> Pedantically, this is undefined (uses of both pos and str may trap after
>> realloc has freed the original pointer), it is better to calculate the
>> difference before calling realloc.
>
>
> And while at it, perhaps it's better to follow the suggestion in
> http://msdn.microsoft.com/en-us/library/aa363679.aspx under Remarks and
> replace "%1" with "%1!S!" instead of "% 1".
>

If my memory serves me correct, we considered this, but found that
it wasn't implemented until Vista. I could be mis-remembering, though.

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

[Andreas Schwab]

Erik Faye-Lund <kusmabite@gmail.com> writes:

> I don't see how it's undefined. It's using the memory that 'pos'
> *points to* that is undefined, no? The difference between 'pos' and
> 'str' should still be the same, it's not like realloc somehow
> magically updates 'pos'...

It does.  Think of segmented architectures, where freeing a pointer
invalidates its segment, so that even loading the value of the pointer
traps.  Probably no such architecture is in use any more, though.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

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

[Junio C Hamano]

Andreas Schwab <schwab@linux-m68k.org> writes:

> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> I don't see how it's undefined. It's using the memory that 'pos'
>> *points to* that is undefined, no? The difference between 'pos' and
>> 'str' should still be the same, it's not like realloc somehow
>> magically updates 'pos'...
>
> It does.  Think of segmented architectures, where freeing a pointer
> invalidates its segment, so that even loading the value of the pointer
> traps.  Probably no such architecture is in use any more, though.

I love seeing that we have somebody who knows and can explain these
dark corners of ANSI C standard ;-)

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

[Erik Faye-Lund]

On Wed, Aug 21, 2013 at 1:01 AM, Andreas Schwab <schwab@linux-m68k.org> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> I don't see how it's undefined. It's using the memory that 'pos'
>> *points to* that is undefined, no? The difference between 'pos' and
>> 'str' should still be the same, it's not like realloc somehow
>> magically updates 'pos'...
>
> It does.  Think of segmented architectures, where freeing a pointer
> invalidates its segment, so that even loading the value of the pointer
> traps.  Probably no such architecture is in use any more, though.
>

Wow, you're right. And since doing it the right way is pretty much the same
complexity (and possibly even a bit easier to follow), that's probably the
best thing to go with, then. Thanks for keeping me straight!

Re: CPPCheck found 24 high risk bugs in Git v.1.8.3.4 (fetch.c L588)

[Philip Oakley]

From: "Philip Oakley" <philipoakley@iee.org>
Sent: Monday, August 19, 2013 10:46 PM
> From: "Koch, Rick (Subcontractor)" <Rick.Koch@tbe.com>
>
>> Ran CPPCheck 1.5.6 on Windows-XP.
>
> Hi Rick,
> Thank you for the clarification.
> Normal practice on the list is to use Reply All, so everyone can 
> participate in the discussion.
>
> It looks like most of the reports are false positives. My bikeshedding 
> thought would be that it is common in Git to inspect all the call 
> sites such that they don't create the various problems, rather than 
> protect against the problems within the various functions, which may 
> be a cause of the reports (i.e. different philosophical approach to 
> checking).
>

I have double checked the reported:
    "wrongPrintfScanfArgNum(CppCheck) \git-master\builtin\fetch.c
    fetch.c 588".

                        fprintf(stderr, " x %-*s %-*s -> %s\n",
                                TRANSPORT_SUMMARY(_("[deleted]")),
                                REFCOL_WIDTH, _("(none)"), 
prettify_refname(ref->name));


At first it did look like there were not enough parameters to satisfy 
the "%-*s" format strings, given that the second invocation has an 
obvious width. This is the only usage within the prune_refs function.

A little further looking shows that the "%-*s" format is used 
extensively in the wider fetch.c and that the TRANSPORT_SUMMARY(), macro 
returns two values as required by the fprintf.

Inaddition  those other invocations aren't flagged showing that this is 
a false positive, and is a good example for feeding back to CPPCheck (If 
you wish Rick) as an example so they can see what went wrong.

Does CPPCheck give more details of 'why' it thinks the other faults are 
present? (e.g. the double pointer checks which can be tricky)

> regards
>
> Philip
> ---
>
> v/r
>
> Roderick (Rick) Koch
> OSF - Information Assurance
> Team Teledyne / Sentar Inc.
> Work: 256-726-1253
> Rick.Koch@tbe.com
>
>
> -----Original Message-----
> From: Philip Oakley [mailto:philipoakley@iee.org]
>
> From: "Koch, Rick (Subcontractor)" <Rick.Koch@tbe.com>
> Sent: Monday, August 19, 2013 6:09 PM
>>I'm directing to this e-mail, as it seems to be the approved forum for
>>posting Git bugs. We ran CPPCheck against Git v.1.8.3.4 and found 24
>>high risk bugs. Please see the attachment xlsx.
>
>>Is there a method to post to the Git community to allow the community
>>to review and debunk as faults positive or develop patches to fix 
>>lists
>>code files?
>
>>v/r
>
>>Roderick (Rick) Koch
>>Information Assurance
>>Rick.Koch@tbe.com
>
> What OS version / CPPCheck version was this checked on?
>
> 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.
>
> Philip
>
> description resourceFilePath fileName lineNumber
>      nullPointer(CppCheck) \git-master\builtin\add.c add.c 286
>      wrongPrintfScanfArgNum(CppCheck) \git-master\builtin\fetch.c 
> fetch.c 588

False positive.

>      nullPointer(CppCheck) \git-master\builtin\ls-files.c ls-files.c
> 144
>      nullPointer(CppCheck) \git-master\builtin\merge.c merge.c 1208
>      doubleFree(CppCheck) \git-master\builtin\notes.c notes.c 275
>      nullPointer(CppCheck) \git-master\builtin\reflog.c reflog.c 437
>      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
>      uninitvar(CppCheck) \git-master\builtin\rev-list.c rev-list.c 342
>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
> 2803
>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
> 2802
>      uninitvar(CppCheck) \git-master\compat\regex\regcomp.c regcomp.c
> 2805
>      memleakOnRealloc(CppCheck) \git-master\compat\win32\syslog.c 
> syslog.c 46

True report.

>      uninitvar(CppCheck)
> \git-master\contrib\examples\builtin-fetch--tool.c 
> builtin-fetch--tool.c
> 419
>      uninitvar(CppCheck) \git-master\fast-import.c fast-import.c 2917
>      nullPointer(CppCheck) \git-master\line-log.c line-log.c 638
>      nullPointer(CppCheck) \git-master\mailmap.c mailmap.c 156
>      uninitvar(CppCheck) \git-master\merge-recursive.c 
> merge-recursive.c 1887
>      uninitvar(CppCheck) \git-master\notes.c notes.c 805
>      uninitvar(CppCheck) \git-master\notes.c notes.c 805
>      deallocret(CppCheck) \git-master\pretty.c pretty.c 677
>      resourceLeak(CppCheck) \git-master\refs.c refs.c 3041
>      doubleFree(CppCheck) \git-master\sequencer.c sequencer.c 924
>      nullPointer(CppCheck) \git-master\sha1_file.c sha1_file.c 125
>      doubleFree(CppCheck) \git-master\shell.c shell.c 130
>
>
> --
Philip