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

[Philip Oakley]

From: Philip Oakley philipoak...@iee.org
Sent: Monday, August 19, 2013 10:46 PM

From: Koch, Rick (Subcontractor) rick.k...@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.k...@tbe.com


-Original Message-
From: Philip Oakley [mailto:philipoak...@iee.org]

From: Koch, Rick (Subcontractor) rick.k...@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.k...@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 


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andreas Schwab]

Erik Faye-Lund kusmab...@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, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 kusmab...@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é

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 sch...@linux-m68k.org wrote:
 Erik Faye-Lund kusmab...@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'...
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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@web.de wrote:
 Am 20.08.2013 20:44, schrieb Andreas Schwab:

 Erik Faye-Lund kusmab...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andreas Schwab]

Erik Faye-Lund kusmab...@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, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Andreas Schwab sch...@linux-m68k.org writes:

 Erik Faye-Lund kusmab...@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 ;-)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 sch...@linux-m68k.org wrote:
 Erik Faye-Lund kusmab...@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!
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Philip Oakley]

From: Koch, Rick (Subcontractor) rick.k...@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.k...@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

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Jeff King p...@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.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Stefan Beller]

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



signature.asc
Description: OpenPGP digital signature

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

[Philip Oakley]

From: Koch, Rick (Subcontractor) rick.k...@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.k...@tbe.com


-Original Message-
From: Philip Oakley [mailto:philipoak...@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.k...@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.k...@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


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Philip Oakley]

- Original Message - 
From: Philip Oakley philipoak...@iee.org

From: Koch, Rick (Subcontractor) rick.k...@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.k...@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 


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 philipoak...@iee.org wrote:
 - Original Message - From: Philip Oakley philipoak...@iee.org

 From: Koch, Rick (Subcontractor) rick.k...@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.k...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html