linux-kernel - Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

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] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.10.1506051813390.2388@hadrien>
Date:	Fri, 5 Jun 2015 18:16:20 +0200 (CEST)
From:	Julia Lawall <julia.lawall@...6.fr>
To:	Fabian Frederick <fabf@...net.be>
cc:	Nicolas Palix <nicolas.palix@...g.fr>,
	Michal Marek <mmarek@...e.cz>,
	Gilles Muller <Gilles.Muller@...6.fr>,
	linux-kernel@...r.kernel.org, cocci@...teme.lip6.fr
Subject: Re: [PATCH 1/1] scripts/coccinelle/misc: add swap.cocci

On Fri, 5 Jun 2015, Fabian Frederick wrote:

>
>
> > On 31 May 2015 at 11:42 Julia Lawall <julia.lawall@...6.fr> wrote:
> >
> >
> > I propose the extended version below (not currently coccicheck friendly). 
> > the extra features are:
> >
> > 1.  The original version requires i1 and i2 to be identifiers, eg x and y. 
> > This doesn't address the case where they are terms line x->a or x[b].  The
> > fact that the original code contains assignments with both i1 and i2 on
> > the left hand side is enough to ensure that they are appropriate arguments
> > for swap.  So they can be changed to expression metavariables.
> >
> > 2. The original patch rule always removed the tmp variable.  This is not
> > valid if the tmp variable is used for something else.  The new semantic
> > patch separates the introduction of swap (rule r) from the removal of the
> > variable declaration (rule ok and the one folowing).  The rule ok checks
> > that this is a function containing an introduced call to swap, and then
> > the rule after removes the declaration if the variable is not used for
> > anything else.  Note that the name of the tmp variable is remembered in
> > the invalid three-argument version of sawp.  This is then cleaned up in
> > the rule below.
> >
> > 3. The original patch always removed the initialization of the tmp
> > variable.  Actually, some code uses the tmp variable afterwards to refer
> > to the old value.  In the new semantic patch, the first set of rules
> > considers the cases where the tmp variable is not used, and the last rule
> > is for the case where the tmp variable is stll needed.  No cleaning up of
> > the declaration is needed in that case.
> >
> > There is one regression as compared to the original semantic patch: In the
> > file lib/mpi/mpi-pow.c, the temporary variable is not needed after the
> > change, but it is also not removed.  It is declared within a loop, and
> > Coccinelle does not realize that it is not needed afterwards, because it
> > is needed on subsequent loop iterations.  Trying to adjust the semantic
> > patch to address this issue made it much slower and didn't fix the
> > problem.  Perhaps it is easier to rely on gcc to give an unused variable
> > warning, and to clean it up then.
> >
> > Fabian, if you are o with this, do you want to sgenify it ans submit a new
> > patch?
>
> Hello Julia,
>
>         Interesting improvements :) Do we really need tmp variable in swap() ?
> I tried the version below which removes swap(x,y,tmp)->swap(x,y) rule
> and had the same result on drivers branch plus it solved a missing tmp.
> Maybe you want to avoid out of context swap() calls ?
>
> Regards,
> Fabian
>
> @r@
> expression i1, i2, E;
> identifier tmp;
> @@
>
> - tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
>   ... when != tmp
> ? tmp = E
>
> @ok exists@
> type t1;
> identifier r.tmp;
> expression i1,i2;
> position p;
> @@
>
> t1@p tmp;
> ...
> swap(i1, i2);

There is no connection between tmp and swap here.  Tmp does have the same
name as some variable used in a previous swap, but there is no guarantee
that this swap here is the one that was introduced by the previous rule.
Unfortunately, when you make a transformation, all information about
positions is lost.  So there is no way to put a position variable on a
generated swap and ensure that the one matched here is exactly the same.

Maybe you could go a little closer to ensuring that property by inheriting
i1 and i2 as well, and not just tmp.

julia

>
> @@
> expression i1,i2;
> identifier tmp;
> type t1;
> position ok.p;
> @@
>
> -t1@p tmp;
>  <... when strict
>       when != tmp
>  swap(i1, i2);
>  ...>
>
>
> // tmp variable still needed
>
> @@
> expression i1, i2;
> identifier tmp;
> @@
>
>   tmp = i1;
> - i1 = i2;
> - i2 = tmp;
> + swap(i1, i2);
>
>
> >
> > thanks,
> > julia
> >
> > // it may be possible to remove the tmp variable
> >
> > @r@
> > expression i1, i2, E;
> > identifier tmp;
> > @@
> >
> > - tmp = i1;
> > - i1 = i2;
> > - i2 = tmp;
> > + swap(i1, i2, tmp);
> >   ... when != tmp
> > ? tmp = E
> >
> > @ok exists@
> > type t1;
> > identifier r.tmp;
> > expression i1,i2;
> > position p;
> > @@
> >
> > t1@p tmp;
> > ...
> > swap(i1, i2, tmp);
> >
> > @@
> > expression i1,i2;
> > identifier tmp;
> > type t1;
> > position ok.p;
> > @@
> >
> > -t1@p tmp;
> >  <... when strict
> >       when != tmp
> >  swap(i1, i2, tmp);
> >  ...>
> >
> > @depends on r@
> > expression i1,i2;
> > identifier tmp;
> > @@
> >
> > swap(i1,i2
> > - ,tmp
> >  )
> >
> > // tmp variable still needed
> >
> > @@
> > expression i1, i2;
> > identifier tmp;
> > @@
> >
> >   tmp = i1;
> > - i1 = i2;
> > - i2 = tmp;
> > + swap(i1, i2);
>