CPPCheck found 24 high risk bugs in Git v.1.8.3.4

> 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

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

> 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

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

> 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] = ' ';
>   }

> 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);

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

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

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

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

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

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