From: Koch, Rick (Subcontractor) <hidden> Date: 2016-06-15 22:58:27
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
From: Philip Oakley <hidden> Date: 2016-06-15 22:58:27
From: "Koch, Rick (Subcontractor)" <redacted>
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
From: Jeff King <hidden> Date: 2016-06-15 22:58:27
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?
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...
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
From: Stefan Beller <hidden> Date: 2016-06-15 22:58:27
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
From: Philip Oakley <hidden> Date: 2016-06-15 22:58:27
----- Original Message -----
From: "Philip Oakley" <redacted>
From: "Koch, Rick (Subcontractor)" <redacted>
Sent: Monday, August 19, 2013 6:09 PM
quoted
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.
quoted
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?
quoted
v/r
quoted
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.
From: Erik Faye-Lund <hidden> Date: 2016-06-15 22:58:27
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)
On Tue, Aug 20, 2013 at 12:55 AM, Philip Oakley [off-list ref] wrote:
----- Original Message ----- From: "Philip Oakley" [off-list ref]
quoted
From: "Koch, Rick (Subcontractor)" <redacted>
Sent: Monday, August 19, 2013 6:09 PM
quoted
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.
quoted
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?
quoted
v/r
quoted
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.
From: Jeff King <hidden> Date: 2016-06-15 22:58:27
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.
quoted hunk
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)
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