[PATCH V3] scripts/coccinelle/free: add NULL test before freeing functions

[PATCH V3] scripts/coccinelle/free: add NULL test before freeing functions

[Fabian Frederick]

Warns or generates patch for NULL check before the following functions:

kfree
usb_free_urb
debugfs_remove
debugfs_remove_recursive

Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Joe Perches <joe@perches.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Fabian Frederick <fabf@skynet.be>
---

V3: 
   -Update print_main message.
   -Add patch mode (suggested by Julia Lawall)

V2:
   -Add 3 more functions to kfree (suggested by Joe Perches)
   -Update warning message to involve code analysis (suggested by Julia Lawall)

 scripts/coccinelle/free/ifnullfree.cocci | 53 ++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)
 create mode 100644 scripts/coccinelle/free/ifnullfree.cocci

diff --git a/scripts/coccinelle/free/ifnullfree.cocci b/scripts/coccinelle/free/ifnullfree.cocci
new file mode 100644
index 0000000..c826d98
--- /dev/null
+++ b/scripts/coccinelle/free/ifnullfree.cocci
@@ -0,0 +1,53 @@
+/// NULL check before some freeing functions is not needed.
+///
+/// Based on checkpatch warning
+/// "kfree(NULL) is safe this check is probably not required"
+/// and kfreeaddr.cocci by Julia Lawall.
+///
+/// Comments: -
+/// Options: --no-includes --include-headers
+
+virtual patch
+virtual org
+virtual report
+virtual context
+
+@r2 depends on patch@
+expression E;
+@@
+- if (E)
+(
+-	kfree(E);
++ kfree(E);
+|
+-	debugfs_remove(E);
++ debugfs_remove(E);
+|
+-	debugfs_remove_recursive(E);
++ debugfs_remove_recursive(E);
+|
+-	usb_free_urb(E);
++ usb_free_urb(E);
+)
+
+@r depends on context || report || org @
+expression E;
+position p;
+@@
+
+* if (E)
+*	\(kfree@p\|debugfs_remove@p\|debugfs_remove_recursive@p\|usb_free_urb\)(E);
+
+@script:python depends on org@
+p << r.p;
+@@
+
+cocci.print_main("NULL check before that freeing function is not needed", p)
+
+@script:python depends on report@
+p << r.p;
+@@
+
+msg = "WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values."
+coccilib.report.print_report(p[0], msg)
+
-- 
1.8.4.5

Re: [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[SF Markus Elfring]

> V3: 
>    -Update print_main message.

Does the discussion topic need also an adjustment?

How do you think about my previous update suggestion "Deletion of
unnecessary checks before specific function calls"?
https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html
https://lkml.org/lkml/2014/3/5/344

Regards,
Markus

Re: [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[Fabian Frederick]

On Sat, 28 Jun 2014 11:08:35 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> 
> > V3: 
> >    -Update print_main message.
> 
> Does the discussion topic need also an adjustment?
> 
> How do you think about my previous update suggestion "Deletion of
> unnecessary checks before specific function calls"?
> https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html
> https://lkml.org/lkml/2014/3/5/344

Hello Markus,

    I didn't see you made the same kind of script.
Ok, you can send yours and we forget this one :)

Regards,
Fabian


> 
> Regards,
> Markus

Re: [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[SF Markus Elfring]

>> How do you think about my previous update suggestion "Deletion of
>> unnecessary checks before specific function calls"?
>> https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html
>> https://lkml.org/lkml/2014/3/5/344
>     I didn't see you made the same kind of script.

Will my approach become a bit more complete in the near future?
http://article.gmane.org/gmane.comp.version-control.coccinelle/3512/

Regards,
Markus

Re: [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[Fabian Frederick]

On Sat, 28 Jun 2014 13:38:15 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> 
> >> How do you think about my previous update suggestion "Deletion of
> >> unnecessary checks before specific function calls"?
> >> https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html
> >> https://lkml.org/lkml/2014/3/5/344
> >     I didn't see you made the same kind of script.
> 
> Will my approach become a bit more complete in the near future?
> http://article.gmane.org/gmane.comp.version-control.coccinelle/3512/
> 

Sorry but I'm not working on coccinelle ; just did a small script for kernel tree. I guess you'll have an answer from coccinelle people.

Regards,
Fabian

> Regards,
> Markus

Re: [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[SF Markus Elfring]

> Sorry but I'm not working on coccinelle ; just did a small script for kernel tree.

I would appreciate a more constructive feedback for my update suggestions around
the topic "Deletion of unnecessary checks before specific function calls".
- Did you look at the concrete patches?
- How do you think about the general approach?

Examples:
https://systeme.lip6.fr/pipermail/cocci/2014-March/000676.html
http://marc.info/?l=kernel-janitors&m=139405971927100&w=2

https://systeme.lip6.fr/pipermail/cocci/2014-March/000677.html
http://marc.info/?l=kernel-janitors&m=139405983727148&w=2

Regards,
Markus

Re: [Cocci] [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[Julia Lawall]

On Sat, 28 Jun 2014, SF Markus Elfring wrote:

> > Sorry but I'm not working on coccinelle ; just did a small script for kernel tree.
> 
> I would appreciate a more constructive feedback for my update suggestions around
> the topic "Deletion of unnecessary checks before specific function calls".
> - Did you look at the concrete patches?
> - How do you think about the general approach?
> 
> Examples:
> https://systeme.lip6.fr/pipermail/cocci/2014-March/000676.html
> http://marc.info/?l=kernel-janitors&m=139405971927100&w=2

Regular expressions do not allow taking advantage of the optimizations 
provided by Coccinelle and are not easy for a reader to understand.

> https://systeme.lip6.fr/pipermail/cocci/2014-March/000677.html
> http://marc.info/?l=kernel-janitors&m=139405983727148&w=2

This is not safe due to the use of when any.  You have no guarantee that 
there is not a dereference of input in the matched region, or that the 
value of input is still the argument value at the point of the test.

julia

Re: [Cocci] [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[SF Markus Elfring]

>> https://systeme.lip6.fr/pipermail/cocci/2014-March/000676.html
>> http://marc.info/?l=kernel-janitors&m=139405971927100&w=2
> Regular expressions do not allow taking advantage of the optimizations 
> provided by Coccinelle and are not easy for a reader to understand.

I find that the application of regexes is appropriate here.
Would you like to optimise any implementation details from my general
approach?


>> https://systeme.lip6.fr/pipermail/cocci/2014-March/000677.html
>> http://marc.info/?l=kernel-janitors&m=139405983727148&w=2
> This is not safe due to the use of when any.  You have no guarantee that 
> there is not a dereference of input in the matched region, or that the 
> value of input is still the argument value at the point of the test.

Do you suggest to make the desired detection of input parameter
validation a bit safer?

Regards,
Markus

Re: [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[Fabian Frederick]

On Sat, 28 Jun 2014 23:00:22 +0200
SF Markus Elfring <elfring@users.sourceforge.net> wrote:

> > Sorry but I'm not working on coccinelle ; just did a small script for kernel tree.
> 
> I would appreciate a more constructive feedback for my update suggestions around
> the topic "Deletion of unnecessary checks before specific function calls".
> - Did you look at the concrete patches?
> - How do you think about the general approach?
> 
> Examples:
> https://systeme.lip6.fr/pipermail/cocci/2014-March/000676.html
> http://marc.info/?l=kernel-janitors&m=139405971927100&w=2
> 
> https://systeme.lip6.fr/pipermail/cocci/2014-March/000677.html
> http://marc.info/?l=kernel-janitors&m=139405983727148&w=2
> 
> Regards,
> Markus

Hello Markus,

	I hope you had information you wanted from Julia ?
If you want more feedback, maybe you can create a new RFC (Request For Comment) in Linux Kernel mailing.

Regards,
Fabian

Re: [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[SF Markus Elfring]

> 	I hope you had information you wanted from Julia ?

Yes. - I am waiting on another Coccinelle software release which will
contain improvements for my feature requests and bug reports.
I'm curious when I should retry proposed filter patterns.

Regards,
Markus

Re: [Cocci] [PATCH V3] scripts/coccinelle/free: Delete NULL test before freeing functions?

[Julia Lawall]

On Sat, 28 Jun 2014, SF Markus Elfring wrote:

>
> > V3:
> >    -Update print_main message.
>
> Does the discussion topic need also an adjustment?
>
> How do you think about my previous update suggestion "Deletion of
> unnecessary checks before specific function calls"?
> https://systeme.lip6.fr/pipermail/cocci/2014-March/000675.html
> https://lkml.org/lkml/2014/3/5/344

I would prefer to accept Fabian's semantic patch for the moment, as it
considers a more restricted, well thought out, set of functions,

julia

Re: [PATCH V3] scripts/coccinelle/free: add NULL test before freeing functions

[Julia Lawall]

On Sat, 28 Jun 2014, Fabian Frederick wrote:

> Warns or generates patch for NULL check before the following functions:
>
> kfree
> usb_free_urb
> debugfs_remove
> debugfs_remove_recursive
>
> Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> Cc: Gilles Muller <Gilles.Muller@lip6.fr>
> Cc: Joe Perches <joe@perches.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>

Acked-by: Julia Lawall <Julia.Lawall@lip6.fr>

> ---
>
> V3:
>    -Update print_main message.
>    -Add patch mode (suggested by Julia Lawall)
>
> V2:
>    -Add 3 more functions to kfree (suggested by Joe Perches)
>    -Update warning message to involve code analysis (suggested by Julia Lawall)
>
>  scripts/coccinelle/free/ifnullfree.cocci | 53 ++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
>  create mode 100644 scripts/coccinelle/free/ifnullfree.cocci
>
> diff --git a/scripts/coccinelle/free/ifnullfree.cocci b/scripts/coccinelle/free/ifnullfree.cocci
> new file mode 100644
> index 0000000..c826d98
> --- /dev/null
> +++ b/scripts/coccinelle/free/ifnullfree.cocci
> @@ -0,0 +1,53 @@
> +/// NULL check before some freeing functions is not needed.
> +///
> +/// Based on checkpatch warning
> +/// "kfree(NULL) is safe this check is probably not required"
> +/// and kfreeaddr.cocci by Julia Lawall.
> +///
> +/// Comments: -
> +/// Options: --no-includes --include-headers
> +
> +virtual patch
> +virtual org
> +virtual report
> +virtual context
> +
> +@r2 depends on patch@
> +expression E;
> +@@
> +- if (E)
> +(
> +-	kfree(E);
> ++ kfree(E);
> +|
> +-	debugfs_remove(E);
> ++ debugfs_remove(E);
> +|
> +-	debugfs_remove_recursive(E);
> ++ debugfs_remove_recursive(E);
> +|
> +-	usb_free_urb(E);
> ++ usb_free_urb(E);
> +)
> +
> +@r depends on context || report || org @
> +expression E;
> +position p;
> +@@
> +
> +* if (E)
> +*	\(kfree@p\|debugfs_remove@p\|debugfs_remove_recursive@p\|usb_free_urb\)(E);
> +
> +@script:python depends on org@
> +p << r.p;
> +@@
> +
> +cocci.print_main("NULL check before that freeing function is not needed", p)
> +
> +@script:python depends on report@
> +p << r.p;
> +@@
> +
> +msg = "WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values."
> +coccilib.report.print_report(p[0], msg)
> +
> --
> 1.8.4.5
>
>

Re: [PATCH V3] scripts/coccinelle/free: add NULL test before freeing functions

[Julia Lawall]

On Sat, 28 Jun 2014, Julia Lawall wrote:

> On Sat, 28 Jun 2014, Fabian Frederick wrote:
>
> > Warns or generates patch for NULL check before the following functions:
> >
> > kfree
> > usb_free_urb
> > debugfs_remove
> > debugfs_remove_recursive
> >
> > Cc: Julia Lawall <Julia.Lawall@lip6.fr>
> > Cc: Gilles Muller <Gilles.Muller@lip6.fr>
> > Cc: Joe Perches <joe@perches.com>
> > Cc: Andrew Morton <akpm@linux-foundation.org>
> > Signed-off-by: Fabian Frederick <fabf@skynet.be>
>
> Acked-by: Julia Lawall <Julia.Lawall@lip6.fr>
>
> > ---
> >
> > V3:
> >    -Update print_main message.
> >    -Add patch mode (suggested by Julia Lawall)
> >
> > V2:
> >    -Add 3 more functions to kfree (suggested by Joe Perches)
> >    -Update warning message to involve code analysis (suggested by Julia Lawall)
> >
> >  scripts/coccinelle/free/ifnullfree.cocci | 53 ++++++++++++++++++++++++++++++++
> >  1 file changed, 53 insertions(+)
> >  create mode 100644 scripts/coccinelle/free/ifnullfree.cocci
> >
> > diff --git a/scripts/coccinelle/free/ifnullfree.cocci b/scripts/coccinelle/free/ifnullfree.cocci
> > new file mode 100644
> > index 0000000..c826d98
> > --- /dev/null
> > +++ b/scripts/coccinelle/free/ifnullfree.cocci
> > @@ -0,0 +1,53 @@
> > +/// NULL check before some freeing functions is not needed.
> > +///
> > +/// Based on checkpatch warning
> > +/// "kfree(NULL) is safe this check is probably not required"
> > +/// and kfreeaddr.cocci by Julia Lawall.
> > +///
> > +/// Comments: -
> > +/// Options: --no-includes --include-headers
> > +
> > +virtual patch
> > +virtual org
> > +virtual report
> > +virtual context
> > +
> > +@r2 depends on patch@
> > +expression E;
> > +@@
> > +- if (E)
> > +(
> > +-	kfree(E);
> > ++ kfree(E);
> > +|
> > +-	debugfs_remove(E);
> > ++ debugfs_remove(E);
> > +|
> > +-	debugfs_remove_recursive(E);
> > ++ debugfs_remove_recursive(E);
> > +|
> > +-	usb_free_urb(E);
> > ++ usb_free_urb(E);
> > +)
> > +
> > +@r depends on context || report || org @
> > +expression E;
> > +position p;
> > +@@
> > +
> > +* if (E)
> > +*	\(kfree@p\|debugfs_remove@p\|debugfs_remove_recursive@p\|usb_free_urb\)(E);
> > +
> > +@script:python depends on org@
> > +p << r.p;
> > +@@
> > +
> > +cocci.print_main("NULL check before that freeing function is not needed", p)
> > +
> > +@script:python depends on report@
> > +p << r.p;
> > +@@
> > +
> > +msg = "WARNING: NULL check before freeing functions like kfree, debugfs_remove, debugfs_remove_recursive or usb_free_urb is not needed. Maybe consider reorganizing relevant code to avoid passing NULL values."
> > +coccilib.report.print_report(p[0], msg)
> > +
> > --
> > 1.8.4.5
> >
> >
>