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] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 5 Jun 2015 17:32:43 +0200 (CEST)
From:	Fabian Frederick <fabf@...net.be>
To:	Julia Lawall <julia.lawall@...6.fr>
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 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);

@@
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);
--
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/