Re: CPPCheck found 24 high risk bugs in Git v.1.8.3.4 (fetch.c L588)
From: "Philip Oakley" Sent: Monday, August 19, 2013 10:46 PM From: "Koch, Rick (Subcontractor)" 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)" 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
On Wed, Aug 21, 2013 at 1:01 AM, Andreas Schwab wrote: > Erik Faye-Lund 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
Andreas Schwab writes: > Erik Faye-Lund 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 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
On Tue, Aug 20, 2013 at 10:34 PM, René Scharfe wrote: > Am 20.08.2013 20:44, schrieb Andreas Schwab: > >> Erik Faye-Lund 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
On Tue, Aug 20, 2013 at 8:44 PM, Andreas Schwab wrote: > Erik Faye-Lund 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
Am 20.08.2013 20:44, schrieb Andreas Schwab: Erik Faye-Lund 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 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
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
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 wrote: > - Original Message - From: "Philip Oakley" > >> From: "Koch, Rick (Subcontractor)" >> 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
- Original Message - From: "Philip Oakley" From: "Koch, Rick (Subcontractor)" 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
From: "Koch, Rick (Subcontractor)" 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)" 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
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
On Mon, Aug 19, 2013 at 10:46 PM, Junio C Hamano wrote: > Jeff King 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, www.herland.net -- 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 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
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
From: "Koch, Rick (Subcontractor)" 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
CPPCheck found 24 high risk bugs in Git v.1.8.3.4
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 CPPCheck_on_Git.v1.8.3.4.xlsx Description: CPPCheck_on_Git.v1.8.3.4.xlsx