smatch 1.53 released

smatch 1.53 released

[Dan Carpenter]

Smatch is a source code checker for C.  Right now the focus is on checking 
for kernel bugs.

Here is are the instructions for installing and using:
    git clone git://repo.or.cz/smatch.git
    cd smatch
    make
    cd /usr/src/linux
    make C=1 CHECK=/path/to/smatch modules bzImage | tee warns.txt 
    egrep '(warn|error):' warns.txt

Some weeks ago I pushed a bad commit.  I haven't had internet access to 
fix it until now.  Sorry about that.  It's fixed now.

Smatch v1.53 improves handling of compound implications.  So if you have 
code like this:
        aaa = 0;
        if (y)
                aaa = 1;
        if (x)
                aaa = 2;
        if (x && y)
                __smatch_print_value("aaa");  // <-- prints aaa == 2
        else
                __smatch_print_value("aaa");  // <-- prints aaa == 0-2

This version also adds implications for switch statements.
        if (!a && x != 42)
                return;
        switch(x) {
        case 1:
                a->member = 1;  // <--  It's ok to dereference 'a' here.
        case 42:
                a->member = 1;  // <-- smatch prints an error here.

regards,
dan carpenter

Re: smatch 1.53 released

[Andi Kleen]

Dan Carpenter <error27@gmail.com> writes:

> Smatch is a source code checker for C.  Right now the focus is on checking 
> for kernel bugs.

Could you give a quick overview on what kinds of bugs it looks for
and where the limitations are?

Thanks,

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: smatch 1.53 released

[Dan Carpenter]

On 6/2/09, Andi Kleen <andi@firstfloor.org> wrote:
> Dan Carpenter <error27@gmail.com> writes:
>
>> Smatch is a source code checker for C.  Right now the focus is on checking
>>
>> for kernel bugs.
>
> Could you give a quick overview on what kinds of bugs it looks for
> and where the limitations are?
>

It's pretty good at finding locking bugs.  It also checks for double
kfree() bugs, null pointer dereferences and also if you check for null
instead of checking for PTR_ERR().  There is a check for using kfree()
instead of kfree_skb() as well.

The cool thing about smatch is that it's pretty easy to write custom
checks.  It's uses sparse as a C parser so you have to look through
expression.h to figure out how to do the pattern matching.  There is a
small example script which shows how that works.
http://repo.or.cz/w/smatch.git?a=blob;f=check_template.c

So you use sparse to grep the code for locking functions and then you
use set_state() to set the state to "locked".

If your code looks like this:
lock_kernel();
if (foo) {
        unlock_kernel();
} else {
        frob();
}
<--  Here the state can be either "locked" or "unlocked".
Calling get_state() here will tell you that it is state &merged.  You
can use get_possible() to get a list of possible states it could be.

Say later code looked like this:
if (!foo) {
<--  Here the state is "locked" because of the !foo.  Smatch figures
this out automatically, that comes from the "implications" module.  Do
other code checkers do this?  I'm pretty proud of the feature either
way.  :)
}

So basically you grep for locking functions and you set the state
based on that, then you grep for return statements and check that the
state is correct or print an error.  Smatch tracks the code paths in
the background and merges states or sets implied states.

Limitations:  The big limitation is that smatch only does one pass
through the code so loops aren't handled correctly.  Eventually it
will do two passes.

A lot of null dereference false positives come from places where it's
hard to tell if a loop is true at least once.
x = NULL;
while (param--) {
        x = &something;
}
x->member;
Someone reading the code probably knows what param is and that it's
non-zero at the start.  There is a "--assume-loops" option to make
smatch assume loops go through once.

Many of the locking false positives come from places where the unlock
happens in a seperate function.  It should be relatively straight
forward to make a list functions to say that if frob_the_module()
returns -12, or -14 that implies it unlocked a certain lock.  I
haven't done this yet.

Otherwise, it's still very young code.  Ideally smatch would know the
possible values of every variable in a function but right now many
variables just default to &undefined.  The implication code is not as
good as it could be.  Also I don't have a good way to build call trees
yet.  There is a lot of work to do at every level.

Still, it doesn't hurt to run smatch on your code before submitting a
patch.  There is an easy script for this:  kchecker /path/to/code.c.
Some of my accepted kernel patches have had bugs which could have been
caught by the current version of smatch...

regards,
dan carpenter

> Thanks,
>
> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only.
>

Re: smatch 1.53 released

[Andi Kleen]

Thanks for the description.

> So basically you grep for locking functions and you set the state
> based on that, then you grep for return statements and check that the

When you say "you" you mean smatch is doing that on its own
or does the user need to do that manually?

> Many of the locking false positives come from places where the unlock
> happens in a seperate function.  It should be relatively straight
> forward to make a list functions to say that if frob_the_module()
> returns -12, or -14 that implies it unlocked a certain lock.  I
> haven't done this yet.

How would one pass  that list of functions to smatch?

I understand correctly that right now it's not inter procedural in its
analysis?

-Andi

-- 
ak@linux.intel.com -- Speaking for myself only.

Re: smatch 1.53 released

[Dan Carpenter]

On 6/4/09, Andi Kleen <andi@firstfloor.org> wrote:
> Thanks for the description.
>
>> So basically you grep for locking functions and you set the state
>> based on that, then you grep for return statements and check that the
>
> When you say "you" you mean smatch is doing that on its own
> or does the user need to do that manually?
>

I meant if you were writing a check for a different type of bug.  The
checks that I mentioned are the ones I have written already.

There is also a line between the core smatch and the checks.  The core
understands code flow and the checks call set_state() and get_state()
and print error messages.  The core is able to handle any C code but
the checks are project specific.

>> Many of the locking false positives come from places where the unlock
>> happens in a seperate function.  It should be relatively straight
>> forward to make a list functions to say that if frob_the_module()
>> returns -12, or -14 that implies it unlocked a certain lock.  I
>> haven't done this yet.
>
> How would one pass  that list of functions to smatch?
>

You can't right now.  That's the bit that I still need to write.  :/
But as an example, one of the checks has a file smatch_data/ with a
list of functions that allocate memory.  Locking functions would be
the same.

> I understand correctly that right now it's not inter procedural in its
> analysis?
>

The null dereference check is, but none of the other checks are.

When the inter procedural analysis is done, it will mostly be a two
pass system where the first step builds a list of functions and what
they imply and the second pass does the check.  Meaning two kernel
compiles.  Or more likely, people will just use the default lists
which will be updated for -rc1 kernels.

Smatch is still in the very early stages.

regards,
dan carpenter

> -Andi
>
> --
> ak@linux.intel.com -- Speaking for myself only.
>

Re: smatch 1.53 released

[Christian Kujau]

On Thu, 4 Jun 2009, Dan Carpenter wrote:
>> Smatch is a source code checker for C.  Right now the focus is on checking
>> for kernel bugs.

Hm, smatch is pretty noisy. I'm getting hundreds of warnings and errors on 
a randconfig build: http://nerdbynature.de/bits/smatch/

False positives or real issues?

Thanks,
Christian.
-- 
BOFH excuse #182:

endothermal recalibration

Re: smatch 1.53 released

[Dan Carpenter]

On 6/9/09, Christian Kujau <lists@nerdbynature.de> wrote:
> On Thu, 4 Jun 2009, Dan Carpenter wrote:
>>> Smatch is a source code checker for C.  Right now the focus is on
>>> checking
>>> for kernel bugs.
>
> Hm, smatch is pretty noisy. I'm getting hundreds of warnings and errors on
> a randconfig build: http://nerdbynature.de/bits/smatch/
>
> False positives or real issues?
>

If it´s not in drivers/ it´s a false positive.  Otherwise there is a
95% chance it is a false positive.  In an ideal world there would be
no bugs and no false positives.  But it´s easier to fix the bugs
smatch reports than to fix smatch so probably eventually it will be
100% false positives.

Quite a lot of the time, I don´t know the code well enough to say
whether a bug is a false positive or not.  Check the bits that you
care about.  The smatch output format is designed for vim.  "vim
filename.c +lineno"

If there was some way to get rid of the "field" messages that would
reduce the false positive count by 20.
kernel/trace/trace_functions_graph.c +808 print_graph_function(14)
error: dereferencing undefined:  'field'
I don´t see a heuristic to do that.

The rest are smatch bugs.  Code like:
foo->bar = NULL;
foo = frob();
foo->bar->baz = x;  // smatch thinks this is a bug since foo->bar was
set to NULL.

Sometimes running smatch with the --debug option can help to find
where smatch gets it wrong.  kchecker also takes the --debug option.

regards,
dan carpenter

> Thanks,
> Christian.
> --
> BOFH excuse #182:
>
> endothermal recalibration
>