linux-kernel - Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic patch

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Mon, 13 Jun 2011 15:55:15 -0500
From:	Greg Dietsche <gregory.dietsche@....edu>
To:	Julia Lawall <julia@...u.dk>
CC:	Gilles.Muller@...6.fr, npalix.work@...il.com, cocci@...u.dk,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] coccinelle: if (ret) return ret; return ret; semantic
 patch

On 06/13/2011 01:38 PM, Julia Lawall wrote:
> How about:
>
> @@
> identifier f;
> expression ret;
> identifier x;
> @@
>
> (
> - if (likely(x)) return ret;
> |
> - if (\(IS_ERR\|IS_ZERO\|is_ordinal_table\)(x)) return ret;
> |
>    if (<+...f(...)...+>) return ret;
> |
> - if (...) return ret;
> )
> return ret;
>
>    
just curious... i see you usually just write "return ret;" here when 
posting. I've assumed that's because it will 1) work and 2) is close enough.

You'll notice I've been doing:
-return ret;
+return ret;
because it seems to help coccinelle realize that it can get rid of extra 
line feeds - does this make sense - or should i just be doing a "return 
ret"?
> I have put the likely case separate from the other function calls to
> benefit from the isomorphism.  I have restricted the argument to these
> functions to be an identifier so that it won't have any side effects.  It
> doesn't have to be the same as ret though.  The third line keeps all other
> ifs that contain function calls.  The fourth line gets rid of everything
> else.
>
> You could see if this finds all of the cases of your proposed rule and if
> it at least doesn't find anything else that you don't want it to find.
>
>    
I'll try it out this afternoon/evening hopefully.
> julia
>
>    

There are two other issues with the patch that I've noticed. I'll be 
teaching myself more on coccinelle to figure these out. Unless someone 
else wants to jump in :) So far I've read or skimmed a number of paper's 
that have been written on Coccinelle... I find it all very interesting :)

1) sometimes you see this type of code - which i've chosen to ignore for 
now:
if ((ret=XXXXX) < 0)
     return ret;
return ret;

which could just be simplified to:
return XXXXX;

for an example see the function load_firmware in 
sound/pci/echoaudio/echoaudio_dsp.c

2) after the semantic patch has removed an "if (...)return ret;" Quite 
often, but not always, we end up with this:
ret=...;
return ret;

which of course could just become
return ...;


So as you can see the problems are quite similar, but a little different.

Greg

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/