Static Analysis

I’ve run cppcheck ( http://cppcheck.sourceforge.net/ ) on some of the c source.

I think cppcheck finds a few issues, but it is a little beyond me.

It does have a few false positives and style issues aren’t interesting.

Could a more knowledgeable person have a look ?

some of the c source

You need to be more specific about the sources checked, and the issues found.

If cppcheck is a little beyond you (and probably me) I am intrigued why you ran it?

For instance in :\BCM2835Dev\mixed\RiscOS\Sources\Video\Render\SprExtend\c\jdhuff.c

This function near the end of what I have copied appear to access a member off the end of an array

static void allocate_registers(asm_workspace wp, workspace *ws)
{
UNUSED;
regname *r;
// IFDEBUG;)
/ Iterate through all registers and assign register numbers
For registers that must be saved to stack, allocate stack space /
int usedregs¹⁵;
for(int i=0;i<15;i++)
usedregs[i] = 0;
FOR_EACH_REGISTER_NAME®
{
if((r→regno >= 0) && (r→regno < 16))
{
usedregs[r→regno] |= r→flags & REGFLAG_USAGE_MASK;
}
}
/ Allocate in order of importance – global, then per-pixel, then x-loop, etc. /
for(int i=REGFLAG_GLOBAL;i<=REGFLAG_YLOOP;i<<=1)
{
FOR_EACH_REGISTER_NAME®
{
if(!(r→flags & i) || (r→regno != -1))
continue;
/ Look for a free register
We prefer a completely free register, or if none is available, the first one that doesn’t clash with this allocation
Additionally, temporary registers are allocated from the top, non-temporary from the bottom, as I think this may help in some situations /
int regno=-1,nextbest=-1;
int blocking = (r→flags & REGFLAG_USAGE_MASK) | REGFLAG_GLOBAL;
if(r→flags & REGFLAG_TEMPORARY)
{
for(int j=15;j>=0;j—)
{
if(!usedregs[j])
{
regno = j;
break;static void allocate_registers(asm_workspace *wp, workspace *ws)
{
UNUSED;
regname *r;
// IFDEBUG;)
/ Iterate through all registers and assign register numbers
For registers that must be saved to stack, allocate stack space /
int usedregs¹⁵;
for(int i=0;i<15;i++)
usedregs[i] = 0;
FOR_EACH_REGISTER_NAME®
{
if((r→regno >= 0) && (r→regno < 16))
{
usedregs[r→regno] |= r→flags & REGFLAG_USAGE_MASK;
}
}
/ Allocate in order of importance – global, then per-pixel, then x-loop, etc. /
for(int i=REGFLAG_GLOBAL;i<=REGFLAG_YLOOP;i<<=1)
{
FOR_EACH_REGISTER_NAME®
{
if(!(r→flags & i) || (r→regno != -1))
continue;
/ Look for a free register
We prefer a completely free register, or if none is available, the first one that doesn’t clash with this allocation
Additionally, temporary registers are allocated from the top, non-temporary from the bottom, as I think this may help in some situations */
int regno=-1,nextbest=-1;
int blocking = (r→flags & REGFLAG_USAGE_MASK) | REGFLAG_GLOBAL;
if(r→flags & REGFLAG_TEMPORARY)
{
for(int j=15;j>=0;j—)
{
if(!usedregs[j]) // <<<<<<<<<<<<<<
{
regno = j;
break;

In \BCM2835Dev\castle\RiscOS\Sources\FileSys\ImageFS\DOSFS\c\OpsFile.c

at line 561 possibly leaks memaddr?

if((r→regno >= 0) && (r→regno < 16))

I hope there is sanitisation further down so it can’t ever consider R15 to be an available register!

In DOSFS […] at line 561

There are at least 2 other memory leaks in DOSFS once you spot the pattern, such are the perils of copy and paste. There’s also a leak in MSDOStoSTRING, but as that’s trimmed out by the preprocessor your static analysis tool probably skipped it.

There are at least 2 other memory leaks in DOSFS once you spot the pattern

Useful contribution by Dom then.

BCM2835Dev\mixed\RiscOS\Sources\HWSupport\USB\Controllers\DWCDriver\c\port.c
Line 62
Size of pointer ‘c’ used instead of size of its data. This is likely to lead to a buffer overflow. You probably intend to write ‘sizeof(*c)’.

\BCM2835Dev\castle\RiscOS\Sources\FileSys\ImageFS\DOSFS\c\DOSnaming.c
Line 319
Defensive programming: The variable ‘index’ is used as an array index before it is checked that is within limits. This can mean that the array might be accessed out of bounds. Reorder conditions such as ‘(a[i] && i < 10)’ to ‘(i < 10 && a[i])’. That way the array will not be accessed if the index is out of limits.

\BCM2835Dev\castle\RiscOS\Sources\Programmer\RTSupport\c\module.c
Line 230 : The expression ‘(X & 0xf000) == 0×7’ is always false. Check carefully constants and operators used, these errors might be hard to spot sometimes. In case of complex expression it might help to split it to separate expressions.

\BCM2835Dev\castle\RiscOS\Sources\Toolbox\Gadgets\c\TextMan.c
Line 816 : Memory leak: scan.new_lines

\BCM2835Dev\castle\RiscOS\Sources\Apps\Draw\c\DrawGrid.c
Line 295 : Finding the same expression on both sides of an operator is suspicious and might indicate a cut and paste or logic error. Please examine this code carefully to determine if it is correct.
—> if ((clip.y0/yInc) & 1 == 1) /* Row number test */

\BCM2835Dev\castle\RiscOS\Sources\Apps\Draw\c\DrawGrid.c
Line : 113 Either the condition ‘xspace==0’ is redundant or there is division by zero at line 116.
/* Find rounded down x, y coordinates */
if (xspace == 0)
{ x0 = x1 = xx;
dx0 = dx1 = 0;
colnum = x0/xspace;
}

I’ve only cheery picked some issues that look wrong to me. There are many other warnings etc which may need looking into, but is beyond me. cppcheck is free.

I’ve only cheery picked some issues that look wrong to me.

I do admire the optimism inherent in “cheery picking”!

More seriously, I’m with Steve on “Useful contribution”, though it’ll take someone distinctly cleverer than I to do something about it.

What’s the performance of cppcheck like? I’m wondering how hard it would be to extend the shared makefiles to add a static analysis build target. Potentially this could be on Unix (there’s partial support for cross-compilation in the build system) or on RISC OS (if cppcheck is easy enough to get building and isn’t too slow)

cppcheck on my 5 year old laptop does about 500 c files in 90 seconds.

This would be a useful thing to have, if anybody feels up to porting it?

It does indeed seem useful; I ran it on my own apps and found a buffer overflow but it’d certainly be more useful if I didn’t have to use Windows to do it!

With that said, I haven’t looked at the source so no idea how portable it is. [Edit: It claims that “Any C++11 compiler should work.” I have a suspicion that CFront isn’t going to handle this one!]

I have a suspicion that CFront isn’t going to handle this one!

GCC?

Yep, seems to be pretty straightforward to build the CLI tool with GCC – just a couple of minor fixes needed. Potentially we could build the GUI too, but I doubt I’ll try that.

Performance seems to be acceptable.

I’ll do a bit more tweaking and testing and then upload a build somewhere.

http://www.phlamethrower.co.uk/misc2/cppcheck.zip

GCC?

It crossed my mind, but I haven’t really followed it since I’ve used the DDE for my own stuff.

http://www.phlamethrower.co.uk/misc2/cppcheck.zip

Thanks! Will test it out tonight and make sure that it finds the same error as the Windows version did :)

Edit: Yup!

@Jeffrey: Thanks – that will be most useful. Though it is considerably faster on my PC, may be more useful on my Iyonix.

It doesn’t look like there’s any concept of functions which can potentially return null pointers. That’s a bit disappointing. Ideally we’d want to be able to craft a rule for something like _swix which would say “This function may return null. If the return value is non-null, none of the output variables will have been updated”. Maybe I should do a bit of research and see if there’s a static analysis tool which is more suitable (the main developer of cppcheck seems to be against the idea of introducing features that will lead to lots of false-positives, but when checking code for an OS I think a more paranoid tool would be better)

It doesn’t look like there’s any concept of functions which can potentially return null pointers. That’s a bit disappointing.

Just a bit . . . I think there are many of my functions that return NULL if nothing went wrong. This isn’t a problem for us, or for C, so…?

Still, it’ll be useful for looking for other things I’ve screwed up. ;-)

We run a whole load of static analysis on NetSurf: http://ci.netsurf-browser.org/jenkins/view/Static%20Analysis/

CppCheck is good for the front ends that are cross-compiled. For the core code, Clang’s scan-build and particularly Coverity are much better. Not sure if RISC OS would be eligible for the Coverity Scan open-source tool: https://scan.coverity.com/

Not sure if RISC OS would be eligible for the Coverity Scan open-source tool: https://scan.coverity.com/

Probably not – several of the things in their “how to register” FAQ look like they go against assorted clauses of the Castle licence.

And having to submit the source code to a remote server and then wait X amount of time for the report to come back isn’t going to be very useful when dealing with the initial teething troubles.

“Your project license terms must not place restrictions on other software that is distributed along with your project.”

I take it then that they don’t permit GPL licenced software…? ;-)