Re: smatch update / 2.5.64 / kbugs.org

Re: smatch update / 2.5.64 / kbugs.org

[dan carpenter]

From: Oleg Drokin <green@namesys.com>
> > The smatch bugs for kernel 2.5.64 are up.  The 
> > new url for the smatch bug database is http://kbugs.org.  
> 
> Unfortunatelly the bug database does not work. I mean I cannot connect to it.
> 

Crap...  sorry about that, I screwed up.

> This script can produce a lot less false positives with even more custom merge rules.
> Here's the diff that if run on fs/ext3/super.c from current bk tree, produces
> only one true bug. (your version from cvs produces one real bug and two false positives)
> (8 less hits on my default build).

I have uploaded your modifications to CVS.  I'll use it 
on the next kernel release.  The unfree.pl was just a few
modifications to the deference_check.pl so your patch
will cut down on the false positives with that also.

thanks,
dan carpenter


-- 
_______________________________________________
Sign-up for your own FREE Personalized E-mail at Mail.com
http://www.mail.com/?sr=signup

Meet Singles
http://corp.mail.com/lavalife

Re: smatch update / 2.5.64 / kbugs.org

[Oleg Drokin]

Hello!

On Fri, Mar 07, 2003 at 01:45:35AM -0500, dan carpenter wrote:
> > > The smatch bugs for kernel 2.5.64 are up.  The 
> > > new url for the smatch bug database is http://kbugs.org.  
> > Unfortunatelly the bug database does not work. I mean I cannot connect to it.
> Crap...  sorry about that, I screwed up.

> > This script can produce a lot less false positives with even more custom merge rules.
> > Here's the diff that if run on fs/ext3/super.c from current bk tree, produces
> > only one true bug. (your version from cvs produces one real bug and two false positives)
> > (8 less hits on my default build).
> I have uploaded your modifications to CVS.  I'll use it 
> on the next kernel release.  The unfree.pl was just a few
> modifications to the deference_check.pl so your patch
> will cut down on the false positives with that also.

Actually I think these free() checks can be extended a lot, it can detect memory leaks and so on.

I have preliminary working version here, but it was only barely tested (And not tested on real kernel at all).
I will spend more time on it today and then send it in.
Incidentally this new version should also work on non-kernel code too, I think ;)
I think that tracking only negative/NULL returns is not the best way.
My current approach is to add several more states like "exported" which means that this value was added
to externally visible variable. Then upon reaching return/end of function we can see if all the
allocated stuff was freed or exported. Tricky part seems to correctly merge all these states
from different branches of if steatements.

Also is anybody working on "redundant assignments" stuff as described in Standford guys papers?

Bye,
    Oleg

Re: smatch update / 2.5.64 / kbugs.org

[Oleg Drokin]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

Hello!

On Fri, Mar 07, 2003 at 09:53:07AM +0300, Oleg Drokin wrote:

> Actually I think these free() checks can be extended a lot, it can detect memory leaks and so on.

Ok, it took me awhile, but here is much extended version of unfree checker.
Now the biggest source of false positives is assignments to global variables and to arrays
I have more complete list of problematic places at beginning of the script, in case anyone
want to enhance it. Also it should now work with userspace code.
<shameless plug>Funnily enough, even though I started to work on this script hoping to find some
deeply hidden bugs in reiserfs, I ended up finding bugs in other filesystems instead ;)</shameless plug>

It also requires this function in smatch.pm (I see that now you have changed set_state() to allow empty
second argument, so it may be not that strictly needed now).
sub reset_state {
    my $name;
    my $i;

    $name = $_[0];

    foreach $state (@states){
        my $quotedname = quotemeta $name;
        my $temp = pop(@states);
        if ($state->{name} =~ /^$quotedname$/){
            $state->{state} = 0;
            $state->{start_line} = 0;
            return;
        }
        push @states, $temp;
    }
}


Bye,
    Oleg

[-- Attachment #2: unfree-new1.pl --]
[-- Type: application/x-perl, Size: 21580 bytes --]

Re: smatch update / 2.5.64 / kbugs.org

[dan carpenter]

From: Oleg Drokin <green@namesys.com>
 
> Also is anybody working on "redundant assignments" stuff as described in Standford guys papers?
> 
 
I have been planning to write an equivelence module that
would save what variables where equivelent.  For example ...
a = b = kmalloc();
c = a;
... a, b and c are all equivelent.

The redundant assignment check looks for places that 
assign a variable to an equivelent variable.  You would 
need to check for this anyway as part of writing the 
equivelence module.

The equivelence module has other uses as well ...
a = b = kmalloc();
if (!a) {
        return -ENOMEM;
}
b->foo = bar;

Right now the dereference check prints a false positive
on those, but the equivelence module would fix that.

I don't have the "redundant code" paper in front of me, 
so I forget what other types of things they looked for.

regards,
dan carpenter

-- 
_______________________________________________
Sign-up for your own FREE Personalized E-mail at Mail.com
http://www.mail.com/?sr=signup

Meet Singles
http://corp.mail.com/lavalife

Re: smatch update / 2.5.64 / kbugs.org

[dan carpenter]

From: Greg KH <greg@kroah.com>
> What I really need to know is, what are all of the reported errors in a
> specific portion of the kernel tree.  If you give some way to search
> based on filename and path, I think you will find a lot more people
> using the results of this tool.  I know I would :)
> 

You are right of course...  I'll do that tomorrow evenning. :)

> Also, what advantage does signing up for a user account on the kbugs.org
> site give you?
> 

I'm glad you asked.  When you log in then you can moderate 
the bugs as a bug or not a bug.  I am going to make that 
more obvious, by adding "Login to moderate" next to each 
bug when you look at the source.

Tracking bug moderations from one version to the next is 
basically working since 2.5.63.

thanks,
dan carpenter

-- 
_______________________________________________
Sign-up for your own FREE Personalized E-mail at Mail.com
http://www.mail.com/?sr=signup

Meet Singles
http://corp.mail.com/lavalife

smatch update / 2.5.64 / kbugs.org

[dan carpenter]

/*  
 * Smatch is an open source c error checker based 
 * on the papers about the Stanford Checker. 
 * (http://smatch.sf.net)  The documentation on coding 
 * smatch checks has been updated since my last email to 
 * this list. 
 *
 */

The smatch bugs for kernel 2.5.64 are up.  The 
new url for the smatch bug database is http://kbugs.org.  

One new script from Monday was "UnFree."  This check 
looks for variables that aren't freed on the error paths.
http://kbugs.org/cgi-bin/index.py?page=bug_list&script=UnFree&kernel=2.5.64

The bug database is still under heavy construction (ie it
is really bad).  But I hope to improve it some more this
week.  Also I plan to add some documentation etc.

The possible bug count for 2.5.64 was 1018.  Eventually 
this information will all be available in a stats page but 
for now I'll just post the raw SQL...

mysql> select kernelver, script, count(script) from bugs where kernelver = "2.5.64" group by script, kernelver;
+-----------+-------------------+---------------+
| kernelver | script            | count(script) |
+-----------+-------------------+---------------+
| 2.5.64    | Dereference       |           469 |
| 2.5.64    | GFP_DMA           |             7 |
| 2.5.64    | ReleaseRegion     |            14 |
| 2.5.64    | SpinlockUndefined |            44 |
| 2.5.64    | SpinSleepLazy     |             4 |
| 2.5.64    | UncheckedReturn   |           119 |
| 2.5.64    | UnFree            |           333 |
| 2.5.64    | UnreachedCode     |            28 |
+-----------+-------------------+---------------+
8 rows in set (0.07 sec)

regards,
dan carpenter


-- 
_______________________________________________
Sign-up for your own FREE Personalized E-mail at Mail.com
http://www.mail.com/?sr=signup

Meet Singles
http://corp.mail.com/lavalife

Re: smatch update / 2.5.64 / kbugs.org

[Greg KH]

On Thu, Mar 06, 2003 at 02:37:27AM -0500, dan carpenter wrote:
> /*  
>  * Smatch is an open source c error checker based 
>  * on the papers about the Stanford Checker. 
>  * (http://smatch.sf.net)  The documentation on coding 
>  * smatch checks has been updated since my last email to 
>  * this list. 
>  *
>  */
> 
> The smatch bugs for kernel 2.5.64 are up.  The 
> new url for the smatch bug database is http://kbugs.org.  

I really appreciate the work you are doing on this project, but I have a
comment on how the output is being generated.

What I really need to know is, what are all of the reported errors in a
specific portion of the kernel tree.  If you give some way to search
based on filename and path, I think you will find a lot more people
using the results of this tool.  I know I would :)

Also, what advantage does signing up for a user account on the kbugs.org
site give you?

thanks,

greg k-h

Re: smatch update / 2.5.64 / kbugs.org

[Oleg Drokin]

Hello!

On Thu, Mar 06, 2003 at 02:37:27AM -0500, dan carpenter wrote:

> The smatch bugs for kernel 2.5.64 are up.  The 
> new url for the smatch bug database is http://kbugs.org.  

Unfortunatelly the bug database does not work. I mean I cannot connect to it.

> One new script from Monday was "UnFree."  This check 
> looks for variables that aren't freed on the error paths.
> http://kbugs.org/cgi-bin/index.py?page=bug_list&script=UnFree&kernel=2.5.64

This is very interesting project, thanks a lot.
I spent some time on this today.
This script can produce a lot less false positives with even more custom merge rules.
Here's the diff that if run on fs/ext3/super.c from current bk tree, produces
only one true bug. (your version from cvs produces one real bug and two false positives)
(8 less hits on my default build).
Still there is a lot room for improvement of course (And probably I will spend some more time
on it), there are still valid cases left catched:
pointer assigned to some externally visible variable/structure before exit (fs/reiserfs/fix_node.c)
kmalloc only done conditionally (fs/reiserfs/namei.c)
probably more ;)

Also I think with this script it is somewhat easy to catch these cases (well, that needs to be coded of course):
loosing allocated pointer by assigning other value to variable and not storing old value anywhere.

Thank you.

Bye,
    Oleg
--- unfree.pl	Thu Mar  6 13:58:33 2003
+++ unfree-new.pl	Thu Mar  6 18:16:51 2003
@@ -12,6 +12,15 @@
     print get_filename(), " ", get_start_line($var), " ", get_lineno(), " ", get_func_pos(), " $msg\n";
 }
 
+
+sub merge_free_and_null($$$){
+    my ($name, $one, $two) = @_;
+
+    if ( ($one eq "free" && $two eq "null") || ($one eq "null" && $two eq "free") ) {
+	return "free";
+    }
+    return merge_rules($name, $one, $two);
+}
 sub merge_rules($$$){
     my ($name, $one, $two) = @_;
 
@@ -36,7 +45,7 @@
 
     return "undefined";
 }
-add_merge_function("merge_rules");
+add_merge_function("merge_free_and_null");
 
 while ($data = get_data()){
     my $tmp;
@@ -88,6 +97,11 @@
 		    set_true_path($name,"non_null");
 		}
 	    }
+	    if ( ! ($cond =~ /_expr/) && ! ($cond =~ /^compound_cond/) ) {
+		if (get_state ($cond)){
+		    set_true_path($cond,"non_null");
+		}
+	    }
 	}
 	for my $cond (@{$conds{falsepath}}){
 	    if ($cond =~ /eq_expr\(\((.*)\)\(integer_cst\(0\)\)\)/){
@@ -102,6 +116,11 @@
 		    set_false_path($name,"null");
 		}
 	    }
+	    if ( ! ($cond =~ /_expr/) && ! ($cond =~ /^compound_cond/) ) {
+		if (get_state ($cond)){
+		    set_false_path($cond,"null");
+		}
+	    }
 	}
     }elsif ($data =~ /^(do_cond) (.*)/){            # check do while() for thouroughness
 	$conds = split_conds($1);
@@ -119,11 +138,23 @@
 		}
 	    }
 	}
+	if ( ! ($cond =~ /_expr/) && ! ($cond =~ /^compound_cond/) ) {
+	    if (get_state ($cond)){
+		set_state($cond,"null");
+	    }
+	}
     }
     
     if ($data =~ /call_expr\(\(addr_expr function_decl\(kfree\)\)\(tree_list: (.*)\)\)/){
 	my $var = $1;
-	if (get_state($var)){
+	my $state = get_state($var);
+	if ( !($state =~ /non_null/) && $state =~ /null/ ) {
+	    error_msg("Freeing null pointer", $state);
+	}
+	if ( $state =~ /free/ ) {
+	    error_msg("Freeing already freed", $state);
+	}
+	if ($state){
 	    set_state($var, "free");
 	}
     }