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

From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Aug 19, 2013, 23:15 UTC
Message-ID: <CABPQNSZidMv4MEW+SqVm94pX4szw9QW8LFWsvBTOXYH7ezzycw@mail.gmail.com>
In-Reply-To: <166132E40AA54EE387BA9B4558823C55@PhilipOakley>

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