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

From: Philip Oakley <philipoakley@iee.org>
Date: Aug 23, 2013, 19:51 UTC
Message-ID: <0AED97BABBFA43DAAE14B8355C81AE3B@PhilipOakley>
In-Reply-To: <2A02F37CE50C4BE38860C7D36F1F14CC@PhilipOakley>

From: "Philip Oakley" <philipoakley@iee.org>
Sent: Monday, August 19, 2013 10:46 PM

Show 16 quoted lines

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

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)

Show 48 quoted lines

> 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

False positive.

Show 15 quoted lines

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

Show 19 quoted lines

>      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