Coccinelle Archive on lore.kernel.org
 help / color / Atom feed
* [Cocci] [PATCH V2] coccinelle: iterators: Add for_each_child.cocci script
@ 2020-10-12 17:19 Sumera Priyadarsini
  2020-10-13 10:30 ` Julia Lawall
  0 siblings, 1 reply; 8+ messages in thread
From: Sumera Priyadarsini @ 2020-10-12 17:19 UTC (permalink / raw)
  To: cocci
  Cc: Julia.Lawall, michal.lkml, nicolas.palix, linux-kernel, Gilles.Muller

While iterating over child nodes with the for_each functions, if
control is transferred from the middle of the loop, as in the case
of a break or return or goto, there is no decrement in the
reference counter thus ultimately resulting in a memory leak.

Add this script to detect potential memory leaks caused by
the absence of of_node_put() before break, goto, or, return
statements which transfer control outside the loop.

Signed-off-by: Sumera Priyadarsini <sylphrenadin@gmail.com>

----
Changes in V2:
- Add options --include-headers and --no-includes
- Add 'when forall` to rules for break and goto
---
 .../coccinelle/iterators/for_each_child.cocci | 355 ++++++++++++++++++
 1 file changed, 355 insertions(+)
 create mode 100644 scripts/coccinelle/iterators/for_each_child.cocci

diff --git a/scripts/coccinelle/iterators/for_each_child.cocci b/scripts/coccinelle/iterators/for_each_child.cocci
new file mode 100644
index 000000000000..57caf9b8d7a5
--- /dev/null
+++ b/scripts/coccinelle/iterators/for_each_child.cocci
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0-only
+// Adds missing of_node_put() before return/break/goto statement within a for_each iterator for child nodes.
+//# False positives can be due to function calls within the for_each
+//# loop that may encapsulate an of_node_put.
+///
+// Confidence: High
+// Copyright: (C) 2020 Sumera Priyadarsini
+// URL: http://coccinelle.lip6.fr
+// Options: --no-includes --include-headers
+
+virtual patch
+virtual context
+virtual org
+virtual report
+
+@r@
+local idexpression n;
+expression e1,e2;
+iterator name for_each_node_by_name, for_each_node_by_type,
+for_each_compatible_node, for_each_matching_node,
+for_each_matching_node_and_match, for_each_child_of_node,
+for_each_available_child_of_node, for_each_node_with_property;
+iterator i;
+statement S;
+expression list [n1] es;
+@@
+
+(
+(
+for_each_node_by_name(n,e1) S
+|
+for_each_node_by_type(n,e1) S
+|
+for_each_compatible_node(n,e1,e2) S
+|
+for_each_matching_node(n,e1) S
+|
+for_each_matching_node_and_match(n,e1,e2) S
+|
+for_each_child_of_node(e1,n) S
+|
+for_each_available_child_of_node(e1,n) S
+|
+for_each_node_with_property(n,e1) S
+)
+&
+i(es,n,...) S
+)
+
+@ruleone depends on patch && !context && !org && !report@
+
+local idexpression r.n;
+iterator r.i,i1;
+expression e;
+expression list [r.n1] es;
+statement S;
+@@
+
+ i(es,n,...) {
+   ...
+(
+   of_node_put(n);
+|
+   e = n
+|
+   return n;
+|
+   i1(...,n,...) S
+|
++  of_node_put(n);
+?  return ...;
+)
+   ... when any
+ }
+
+@ruletwo depends on patch && !context && !org && !report@
+
+local idexpression r.n;
+iterator r.i,i1,i2;
+expression e,e1;
+expression list [r.n1] es;
+statement S,S2;
+@@
+
+ i(es,n,...) {
+   ...
+(
+   of_node_put(n);
+|
+   e = n
+|
+   i1(...,n,...) S
+|
++  of_node_put(n);
+?  break;
+)
+   ... when any
+ }
+... when != n
+    when strict
+    when forall
+(
+ n = e1;
+|
+?i2(...,n,...) S2
+)
+
+@rulethree depends on patch && !context && !org && !report exists@
+
+local idexpression r.n;
+iterator r.i,i1,i2;
+expression e,e1;
+identifier l;
+expression list [r.n1] es;
+statement S,S2;
+@@
+
+ i(es,n,...) {
+   ...
+(
+   of_node_put(n);
+|
+   e = n
+|
+   i1(...,n,...) S
+|
++  of_node_put(n);
+?  goto l;
+)
+   ... when any
+ }
+... when exists
+l: ... when != n
+       when strict
+       when forall
+(
+ n = e1;
+|
+?i2(...,n,...) S2
+)
+
+// ----------------------------------------------------------------------------
+
+@ruleone_context depends on !patch && (context || org || report) exists@
+statement S;
+expression e;
+expression list[r.n1] es;
+iterator r.i, i1;
+local idexpression r.n;
+position j0, j1;
+@@
+
+ i@j0(es,n,...) {
+   ...
+(
+   of_node_put(n);
+|
+   e = n
+|
+   return n;
+|
+   i1(...,n,...) S
+|
+  return @j1 ...;
+)
+   ... when any
+ }
+
+@ruleone_disj depends on !patch && (context || org || report)@
+expression list[r.n1] es;
+iterator r.i;
+local idexpression r.n;
+position ruleone_context.j0, ruleone_context.j1;
+@@
+
+*  i@j0(es,n,...) {
+   ...
+*return  @j1...;
+   ... when any
+ }
+
+@ruletwo_context depends on !patch && (context || org || report) exists@
+statement S, S2;
+expression e, e1;
+expression list[r.n1] es;
+iterator r.i, i1, i2;
+local idexpression r.n;
+position j0, j2;
+@@
+
+ i@j0(es,n,...) {
+   ...
+(
+   of_node_put(n);
+|
+   e = n
+|
+   i1(...,n,...) S
+|
+  break@j2;
+)
+   ... when any
+ }
+... when != n
+    when strict
+    when forall
+(
+ n = e1;
+|
+?i2(...,n,...) S2
+)
+
+@ruletwo_disj depends on !patch && (context || org || report)@
+statement S2;
+expression e1;
+expression list[r.n1] es;
+iterator r.i, i2;
+local idexpression r.n;
+position ruletwo_context.j0, ruletwo_context.j2;
+@@
+
+*  i@j0(es,n,...) {
+   ...
+*break @j2;
+   ... when any
+ }
+... when != n
+    when strict
+    when forall
+(
+  n = e1;
+|
+?i2(...,n,...) S2
+)
+
+@rulethree_context depends on !patch && (context || org || report) exists@
+identifier l;
+statement S,S2;
+expression e, e1;
+expression list[r.n1] es;
+iterator r.i, i1, i2;
+local idexpression r.n;
+position j0, j3;
+@@
+
+ i@j0(es,n,...) {
+   ...
+(
+   of_node_put(n);
+|
+   e = n
+|
+   i1(...,n,...) S
+|
+  goto l@j3;
+)
+  ... when any
+ }
+... when exists
+l:
+... when != n
+    when strict
+    when forall
+(
+ n = e1;
+|
+?i2(...,n,...) S2
+)
+
+@rulethree_disj depends on !patch && (context || org || report) exists@
+identifier l;
+statement S2;
+expression e1;
+expression list[r.n1] es;
+iterator r.i, i2;
+local idexpression r.n;
+position rulethree_context.j0, rulethree_context.j3;
+@@
+
+*  i@j0(es,n,...) {
+   ...
+*goto l@j3;
+   ... when any
+ }
+... when exists
+ l:
+ ... when != n
+     when strict
+     when forall
+(
+ n = e1;
+|
+?i2(...,n,...) S2
+)
+
+// ----------------------------------------------------------------------------
+
+@script:python ruleone_org depends on org@
+i << r.i;
+j0 << ruleone_context.j0;
+j1 << ruleone_context. j1;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before return " % (i)
+coccilib.org.print_safe_todo(j0[0], msg)
+coccilib.org.print_link(j1[0], "")
+
+@script:python ruletwo_org depends on org@
+i << r.i;
+j0 << ruletwo_context.j0;
+j2 << ruletwo_context.j2;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before break " % (i)
+coccilib.org.print_safe_todo(j0[0], msg)
+coccilib.org.print_link(j2[0], "")
+
+@script:python rulethree_org depends on org@
+i << r.i;
+j0 << rulethree_context.j0;
+j3 << rulethree_context.j3;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before goto " % (i)
+coccilib.org.print_safe_todo(j0[0], msg)
+coccilib.org.print_link(j3[0], "")
+
+// ----------------------------------------------------------------------------
+
+@script:python ruleone_report depends on report@
+i << r.i;
+j0 << ruleone_context.j0;
+j1 << ruleone_context.j1;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before return around line %s." % (i, j1[0].line)
+coccilib.report.print_report(j0[0], msg)
+
+@script:python ruletwo_report depends on report@
+i << r.i;
+j0 << ruletwo_context.j0;
+j2 << ruletwo_context.j2;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before break around line %s." % (i,j2[0].line)
+coccilib.report.print_report(j0[0], msg)
+
+@script:python rulethree_report depends on report@
+i << r.i;
+j0 << rulethree_context.j0;
+j3 << rulethree_context.j3;
+@@
+
+msg = "WARNING: Function \"%s\" should have of_node_put() before goto around lines %s." % (i,j3[0].line)
+coccilib.report.print_report(j0[0], msg)
-- 
2.25.1

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] [PATCH V2] coccinelle: iterators: Add for_each_child.cocci script
  2020-10-12 17:19 [Cocci] [PATCH V2] coccinelle: iterators: Add for_each_child.cocci script Sumera Priyadarsini
@ 2020-10-13 10:30 ` Julia Lawall
  0 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2020-10-13 10:30 UTC (permalink / raw)
  To: Sumera Priyadarsini
  Cc: Gilles.Muller, michal.lkml, nicolas.palix, cocci, linux-kernel

I have one more change to suggest.  This one only affects the patch case,
as the other cases just point to a problem but don't ompletely specify
what to do about it.

> +@ruleone depends on patch && !context && !org && !report@
> +
> +local idexpression r.n;
> +iterator r.i,i1;
> +expression e;
> +expression list [r.n1] es;
> +statement S;
> +@@
> +
> + i(es,n,...) {
> +   ...
> +(
> +   of_node_put(n);
> +|
> +   e = n
> +|
> +   return n;
> +|
> +   i1(...,n,...) S
> +|
> ++  of_node_put(n);
> +?  return ...;
> +)
> +   ... when any
> + }

There is one occurrence of the following code:

        for_each_available_child_of_node(search, child) {
                name = of_get_property(child, "regulator-compatible", NULL);
                if (!name)
                        name = child->name;

                if (!strcmp(desc->of_match, name)) {
                        of_node_put(search);
                        return of_node_get(child);
                }
        }

In this case, the for_each_available_child_of_node has incremented the
reference count of child by 1, and then the return increments it again.
It would be ok to put an of_not_put before the return, which is done by
the above rule, but the code would be even simpler if it would just leave
the reference count as is.  So before the current return case, you can put
another return case that does the following:

-  return of_node_get(n);
+  return n;

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script
       [not found]                   ` <trinity-ceb5e170-ec1a-496c-94bf-eec0c6c4b715-1602654627679@3c-app-webde-bs05>
@ 2020-10-14  6:18                     ` Julia Lawall
  0 siblings, 0 replies; 8+ messages in thread
From: Julia Lawall @ 2020-10-14  6:18 UTC (permalink / raw)
  To: Markus Elfring; +Cc: Michal Marek, Gilles Muller, Nicolas Palix, Coccinelle


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



On Wed, 14 Oct 2020, Markus Elfring wrote:

> > > Is short-circuit evaluation applied for a disjunction (according to macro calls)
> > > in the discussed SmPL rule “r”?
> >
> > The short circuit evaluation is applied at the level of CFG nodes.
> > For each CFG node, it tries the sequence of patters one by one until it finds
> > one that matches.
>
> Do you acknowledge then that disjunctions of the semantic patch language
> work in the way so far that subsequent condition parts are not checked any more
> after a match was determined for a specific source code search pattern?

Yes, at the CFG node level.

>
> Is this functionality equivalent to the operation “ordered choice”?
>
> Is another communication difficulty resolved now according to your hint
> that macro calls would be disjoint (and distinct)?
>
>
> Would you expect that the occurrences of the mentioned identifiers are different
> in the analysed source files?

Sure.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script
       [not found]               ` <trinity-991d9da6-7a4f-4f30-a645-5f4d54b80c53-1602620539556@3c-app-webde-bs30>
@ 2020-10-13 20:24                 ` Julia Lawall
       [not found]                   ` <trinity-ceb5e170-ec1a-496c-94bf-eec0c6c4b715-1602654627679@3c-app-webde-bs05>
  0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2020-10-13 20:24 UTC (permalink / raw)
  To: Markus Elfring; +Cc: Nicolas Palix, Michal Marek, Coccinelle, Gilles Muller


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



On Tue, 13 Oct 2020, Markus Elfring wrote:

> > The rule is applied independently to each node in the control-flow graph.
>
> Please explain a specific technical detail once more.
> Is short-circuit evaluation applied for a disjunction (according to macro calls)
> in the discussed SmPL rule “r”?

The short circuit evaluation is applied at the level of CFG nodes.  For
each CFG node, it tries the sequence of patters one by one until it finds
one that matches.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script
       [not found]           ` <trinity-a1f3e64f-e955-45f2-8ee7-0a26c2974e38-1602614175986@3c-app-webde-bs30>
@ 2020-10-13 19:01             ` Julia Lawall
       [not found]               ` <trinity-991d9da6-7a4f-4f30-a645-5f4d54b80c53-1602620539556@3c-app-webde-bs30>
  0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2020-10-13 19:01 UTC (permalink / raw)
  To: Markus Elfring; +Cc: Nicolas Palix, Michal Marek, Coccinelle, Gilles Muller



On Tue, 13 Oct 2020, Markus Elfring wrote:

> > >   The sorting of macro calls according to an estimated or actual usage frequency
> > >   can influence the evaluation characteristics of affected SmPL code,
> > >   can't it?
> >
> > No.  As I already pointed out, the different macros are disjoint.
>
> This information can be reasonable.
>
>
> > The order doens't matter.
>
> I wonder about such a feedback.
>
> How does it fit to the functionality of SmPL disjunctions?
> Is short-circuit evaluation applied for them?

The rule is applied independently to each node in the control-flow graph.
An example of a node is a loop header.  Any given loop header can match
only one of the provided patterns.  So the order doesn't matter.

julia

>
>
> > Only one of the patterns will match any given loop.
>
> This is generally fine.
> Will their occurrences vary in significant ways in the analysed source code?
>
> Regards,
> Markus
>
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script
       [not found]       ` <trinity-add9d6f9-2889-4bf6-97b3-83add07516ff-1602607555446@3c-app-webde-bs30>
@ 2020-10-13 16:53         ` Julia Lawall
       [not found]           ` <trinity-a1f3e64f-e955-45f2-8ee7-0a26c2974e38-1602614175986@3c-app-webde-bs30>
  0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2020-10-13 16:53 UTC (permalink / raw)
  To: Markus Elfring; +Cc: Nicolas Palix, Michal Marek, Coccinelle, Gilles Muller



On Tue, 13 Oct 2020, Markus Elfring wrote:

> > > Would you care a bit more for the clarification of the ordering for the shown macro names?
> >
> > Why does the ordering matter, since they are all distinct?
>
> * It might look promising to reorder macro calls according to name criteria
>   and passed parameters.
>
> * But I imagine that the functionality of disjunctions by the semantic patch language
>   can trigger further development considerations more in another direction.
>   https://github.com/coccinelle/coccinelle/blob/730dbb034559b3e549ec0b2973cd0400a3fa072f/docs/manual/cocci_syntax.tex#L1033
>
>   Later source code search patterns will only be checked in such SmPL disjunctions
>   if previous parts did not match.
>   Thus often used code variants should probably be specified at the beginning
>   while special selections should be moved to the end.
>   The sorting of macro calls according to an estimated or actual usage frequency
>   can influence the evaluation characteristics of affected SmPL code,
>   can't it?

No.  As I already pointed out, the different macros are disjoint.  The
order doens't matter.  Only one of the patterns will match any given loop.
If there are nested loops, the pattern will match multiple times.

julia
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script
       [not found]   ` <trinity-75bb5607-ae06-450d-95a6-fa9cb0aaf732-1602599807925@3c-app-webde-bs30>
@ 2020-10-13 14:48     ` Julia Lawall
       [not found]       ` <trinity-add9d6f9-2889-4bf6-97b3-83add07516ff-1602607555446@3c-app-webde-bs30>
  0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2020-10-13 14:48 UTC (permalink / raw)
  To: Markus Elfring; +Cc: Nicolas Palix, Michal Marek, Coccinelle, Gilles Muller


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



On Tue, 13 Oct 2020, Markus Elfring wrote:

> > > > +(
> > > > +for_each_node_by_name(n,e1) S
> > > > +|
> > > …
> > > > +|
> > > > +for_each_node_with_property(n,e1) S
> > > > +)
> > > …
> > >
> > > I propose to reconsider the coding style for this SmPL disjunction once more.
> > >
> > > Date: Thu, 24 Sep 2020 14:45:25 +0200
> > > https://lore.kernel.org/cocci/5a74b3c5-c0e9-1426-c477-72bb86bcf5ed@web.de/
> >
> > It's quite fine as is.
>
> Would you care a bit more for the clarification of the ordering for the shown macro names?

Why does the ordering matter, since they are all distinct?

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Cocci] [PATCH v2] coccinelle: iterators: Add for_each_child.cocci script
       [not found] <trinity-d2989d61-4401-4280-9989-055536630329-1602595815473@3c-app-webde-bs30>
@ 2020-10-13 13:35 ` Julia Lawall
       [not found]   ` <trinity-75bb5607-ae06-450d-95a6-fa9cb0aaf732-1602599807925@3c-app-webde-bs30>
  0 siblings, 1 reply; 8+ messages in thread
From: Julia Lawall @ 2020-10-13 13:35 UTC (permalink / raw)
  To: Markus Elfring
  Cc: Michal Marek, Gilles Muller, Nicolas Palix, Julia Lawall, Coccinelle


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



On Tue, 13 Oct 2020, Markus Elfring wrote:

> …
> > +(
> > +for_each_node_by_name(n,e1) S
> > +|
> …
> > +|
> > +for_each_node_with_property(n,e1) S
> > +)
> …
>
> I propose to reconsider the coding style for this SmPL disjunction once more.
>
> Date: Thu, 24 Sep 2020 14:45:25 +0200
> https://lore.kernel.org/cocci/5a74b3c5-c0e9-1426-c477-72bb86bcf5ed@web.de/

It's quite fine as is.

julia

[-- Attachment #2: Type: text/plain, Size: 136 bytes --]

_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-12 17:19 [Cocci] [PATCH V2] coccinelle: iterators: Add for_each_child.cocci script Sumera Priyadarsini
2020-10-13 10:30 ` Julia Lawall
     [not found] <trinity-d2989d61-4401-4280-9989-055536630329-1602595815473@3c-app-webde-bs30>
2020-10-13 13:35 ` [Cocci] [PATCH v2] " Julia Lawall
     [not found]   ` <trinity-75bb5607-ae06-450d-95a6-fa9cb0aaf732-1602599807925@3c-app-webde-bs30>
2020-10-13 14:48     ` Julia Lawall
     [not found]       ` <trinity-add9d6f9-2889-4bf6-97b3-83add07516ff-1602607555446@3c-app-webde-bs30>
2020-10-13 16:53         ` Julia Lawall
     [not found]           ` <trinity-a1f3e64f-e955-45f2-8ee7-0a26c2974e38-1602614175986@3c-app-webde-bs30>
2020-10-13 19:01             ` Julia Lawall
     [not found]               ` <trinity-991d9da6-7a4f-4f30-a645-5f4d54b80c53-1602620539556@3c-app-webde-bs30>
2020-10-13 20:24                 ` Julia Lawall
     [not found]                   ` <trinity-ceb5e170-ec1a-496c-94bf-eec0c6c4b715-1602654627679@3c-app-webde-bs05>
2020-10-14  6:18                     ` Julia Lawall

Coccinelle Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/cocci/0 cocci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 cocci cocci/ https://lore.kernel.org/cocci \
		cocci@systeme.lip6.fr
	public-inbox-index cocci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/fr.lip6.systeme.cocci


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git