[PATCH v2] coccinelle: semantic code search for missing of_node_put

[PATCH v2] coccinelle: semantic code search for missing of_node_put

[Wen Yang]

There are functions which increment a reference counter for a device node.
These functions belong to a programming interface for the management
of information from device trees.
The counter must be decremented after the last usage of a device node.
We find these functions by using the following SmPL:

<SmPL>
@initialize:ocaml@
@@

let relevant_str = "use of_node_put() on it when done"

let contains s1 s2 =
    let re = Str.regexp_string s2
    in
        try ignore (Str.search_forward re s1 0); true
        with Not_found -> false

let relevant_functions = ref []

let add_function f c =
    if not (List.mem f !relevant_functions)
    then
      begin
        let s = String.concat " "
          (
            (List.map String.lowercase_ascii
              (List.filter
                (function x ->
                  Str.string_match
                  (Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$")
                x 0) (Str.split (Str.regexp "[ .;\t\n]+") c)))) in
             if contains s relevant_str
             then
               Printf.printf "Found relevant function: %s\n" f;
               relevant_functions := f :: !relevant_functions;
      end

@r@
identifier fn;
comments c;
type T = struct device_node *;
@@

T@c fn(...) {
...
}

@script:ocaml@
f << r.fn;
c << r.c;
@@

let (cb,cm,ca) = List.hd c in
let c = String.concat " " cb in
add_function f c
</SmPL>

And this patch also looks for places where an of_node_put()
call is on some paths but not on others.

Suggested-by: Julia Lawall <Julia.Lawall@lip6.fr>
Signed-off-by: Wen Yang <wen.yang99@zte.com.cn>
Cc: Julia Lawall <Julia.Lawall@lip6.fr>
Cc: Gilles Muller <Gilles.Muller@lip6.fr>
Cc: Nicolas Palix <nicolas.palix@imag.fr>
Cc: Michal Marek <michal.lkml@markovi.net>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Cc: Markus Elfring <Markus.Elfring@web.de>
Cc: cocci@systeme.lip6.fr
---
v2: improve the commit description and delete duplicate code.

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

diff --git a/scripts/coccinelle/free/of_node_put.cocci b/scripts/coccinelle/free/of_node_put.cocci
new file mode 100644
index 0000000..81a9f54
--- /dev/null
+++ b/scripts/coccinelle/free/of_node_put.cocci
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing of_node_put
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@initialize:python@
+@@
+
+seen = set()
+
+def add_if_not_present (p1, p2):
+    if (p1, p2) not in seen:
+        seen.add((p1, p2))
+        return True
+    return False
+
+def display_report(p1, p2):
+    if add_if_not_present(p1[0].line, p2[0].line):
+       coccilib.report.print_report(p2[0],
+                                    "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
+                                    + p1[0].line
+                                    + ", but without a corresponding object release within this function.")
+
+def display_org(p1, p2):
+    cocci.print_main("acquired a node pointer with refcount incremented", p1)
+    cocci.print_secs("needed of_node_put", p2)
+
+@r1 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+statement S;
+type T;
+@@
+
+x = @p1\(of_find_all_nodes\|
+         of_get_cpu_node\|
+         of_get_parent\|
+         of_get_next_parent\|
+         of_get_next_child\|
+         of_get_next_cpu_node\|
+         of_get_compatible_child\|
+         of_get_child_by_name\|
+         of_find_node_opts_by_path\|
+         of_find_node_by_name\|
+         of_find_node_by_type\|
+         of_find_compatible_node\|
+         of_find_node_with_property\|
+         of_find_matching_node_and_match\|
+         of_find_node_by_phandle\|
+         of_parse_phandle\|
+         of_find_next_cache_node\|
+         of_get_next_available_child\)(...);
+...
+if (x == NULL || ...) S
+... when != e = (T)x
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+
+@script:python depends on report && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+display_report(p1, p2)
+
+@script:python depends on org && r1@
+p1 << r1.p1;
+p2 << r1.p2;
+@@
+
+display_org(p1, p2)
+
+@r2 exists@
+local idexpression struct device_node *x;
+expression e, e1;
+position p1, p2;
+identifier f;
+statement S;
+type T;
+@@
+
+(
+x = f@p1(...);
+... when != e = (T)x
+    when != true x == NULL
+    when != of_node_put(x)
+    when != of_get_next_parent(x)
+    when != of_find_matching_node(x, ...)
+    when != if (x) { ... return x; }
+    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
+    when != e1 = of_fwnode_handle(x)
+(
+if (x) { ... when forall
+         of_node_put(x) ... }
+|
+return x;
+|
+return of_fwnode_handle(x);
+|
+return@p2 ...;
+)
+&
+x = f(...)
+...
+if (<+...x...+>) S
+...
+of_node_put(x);
+)
+@script:python depends on report && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+display_report(p1, p2)
+
+@script:python depends on org && r2@
+p1 << r2.p1;
+p2 << r2.p2;
+@@
+
+display_org(p1, p2)
-- 
2.9.5

Re: [PATCH v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

> The counter must be decremented after the last usage of a device node.

Thanks for your next try to improve the software situation
also in this area.


> We find these functions by using the following SmPL:

Would it be nicer to use the word “script” also here?


> <SmPL>
> @initialize:ocaml@
> @@

How do you think about to describe the chosen algorithm
in a way for contributors who might not be so familiar with
this programming language?

Will any information from previous discussions become relevant
for a better commit description?


> let relevant_str = "use of_node_put() on it when done"

Will such a literal need further development and software documentation considerations?


> let contains s1 s2 =
>     let re = Str.regexp_string s2
>     in
>         try ignore (Str.search_forward re s1 0); true
>         with Not_found -> false
>
> let relevant_functions = ref []
>
> let add_function f c =
>     if not (List.mem f !relevant_functions)
>     then
>       begin
>         let s = String.concat " "

I find such a concatenation suspicious after the space character
is used also for a string splitting before.
Can this delimiter be omitted for the combination?


>           (
>             (List.map String.lowercase_ascii
>               (List.filter
>                 (function x ->
>                   Str.string_match
>                   (Str.regexp "[a-zA-Z_\\(\\)][-a-zA-Z0-9_\\(\\)]*$")
>                 x 0) (Str.split (Str.regexp "[ .;\t\n]+") c)))) in
>              if contains s relevant_str

I would prefer to use the string constant in the called function directly
instead of passing it as another parameter.


>              then
>                Printf.printf "Found relevant function: %s\n" f;
>                relevant_functions := f :: !relevant_functions;
>       end

I find your choice for an output format unclear at the moment.
I imagine that the corresponding data processing of these function names
will need fine-tuning.
I am missing the software component for the conversion of this
identifier list into a disjunction for the SmPL rule “r1”.


> And this patch also looks for places where an of_node_put()

Does a patch or a script perform an action?


> call is on some paths but not on others.

Let us look at mentioned implementation details.


> +@initialize:python@
> +@@
> +
> +seen = set()
> +
> +def add_if_not_present (p1, p2):

It seems that you would like to use iteration functionality.
https://github.com/coccinelle/coccinelle/blob/99e081e9b89d49301b7bd2c5e5aac88c66eaaa6a/docs/manual/cocci_syntax.tex#L1826

How will it matter here?


> +def display_report(p1, p2):
> +    if add_if_not_present(p1[0].line, p2[0].line):
> +       coccilib.report.print_report(p2[0],
> +                                    "ERROR: missing of_node_put; acquired a node pointer with refcount incremented on line "
> +                                    + p1[0].line
> +                                    + ", but without a corresponding object release within this function.")
> +
> +def display_org(p1, p2):
> +    cocci.print_main("acquired a node pointer with refcount incremented", p1)
> +    cocci.print_secs("needed of_node_put", p2)

Can it be helpful to specify SmPL dependencies for these functions
according to the applied operation mode?


> +x = @p1\(of_find_all_nodes\|

I would find this SmPL disjunction easier to read without the usage
of extra backslashes.

+x =
+(of_…
+|of_…
+)@p1(...);


Which sort criteria were applied for the generation of the shown
function name list?


> +if (x == NULL || ...) S
> +... when != e = (T)x
> +    when != true x == NULL

I wonder if this code exclusion specification is really required
after a null pointer was checked before.


> +|
> +return x;
> +|
> +return of_fwnode_handle(x);

Can a nested SmPL disjunction be helpful at such places?

+|return
+(x
+|of_fwnode_handle(x)
+);


> +    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)

Would the specification variant “<+... x ...+>” be relevant
for the parameter selection?


> +&
> +x = f(...)
> +...
> +if (<+...x...+>) S
> +...
> +of_node_put(x);

You propose once more to use a SmPL conjunction in the rule “r2”.
How does it fit to the previous exclusion specification “when != of_node_put(x)”?

Regards,
Markus

Re: [PATCH v2] coccinelle: semantic code search for missing of_node_put

[Julia Lawall]

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

> > +x = @p1\(of_find_all_nodes\|
>
> I would find this SmPL disjunction easier to read without the usage
> of extra backslashes.
>
> +x =
> +(of_…
> +|of_…
> +)@p1(...);

Did you actually test this?  I doubt that a position metavariable can be
put on a ) of a disjunction.

> > +|
> > +return x;
> > +|
> > +return of_fwnode_handle(x);
>
> Can a nested SmPL disjunction be helpful at such places?
>
> +|return
> +(x
> +|of_fwnode_handle(x)
> +);

The original code is much more readable.  The internal representation will
be the same.

> > +    when != v4l2_async_notifier_add_fwnode_subdev(<...x...>)
>
> Would the specification variant “<+... x ...+>” be relevant
> for the parameter selection?

I'm indeed quite surprised that <...x...> would be accepted by the parser.

julia

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> +x =
>> +(of_…
>> +|of_…
>> +)@p1(...);
>
> Did you actually test this?  I doubt that a position metavariable can be
> put on a ) of a disjunction.

Would you ever like to support this possibility?


>> +|return
>> +(x
>> +|of_fwnode_handle(x)
>> +);
>
> The original code is much more readable.

We have got different views around such specification variants.


> The internal representation will be the same.

I imagine that the Coccinelle software might evolve into additional directions.


Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> +x =
>> +(of_…
>> +|of_…
>> +)@p1(...);
>
> Did you actually test this?  I doubt that a position metavariable can be
> put on a ) of a disjunction.

Would you ever like to support this possibility?


>> +|return
>> +(x
>> +|of_fwnode_handle(x)
>> +);
>
> The original code is much more readable.

We have got different views around such specification variants.


> The internal representation will be the same.

I imagine that the Coccinelle software might evolve into additional directions.


Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>> +if (x == NULL || ...) S
>> +... when != e = (T)x
>> +    when != true x == NULL
>
> I wonder if this code exclusion specification is really required
> after a null pointer was checked before.

I would like to add another view for this implementation detail.

The when constraint can express a software desire which can be reasonable
to some degree. You would like to be sure that a null pointer will not occur
after a corresponding check succeeded.

* But I feel unsure about the circumstances under which the Coccinelle software
  can determine this aspect actually.

* I find that it can eventually make sense only after the content of
  the local variable (which is identified by “x”) was modified.
  Thus I would find the exclusion of assignments more useful at this place.

Regards,
Markus

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Julia Lawall]

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



On Sat, 29 Jun 2019, Markus Elfring wrote:

> >> +if (x == NULL || ...) S
> >> +... when != e = (T)x
> >> +    when != true x == NULL
> >
> > I wonder if this code exclusion specification is really required
> > after a null pointer was checked before.
>
> I would like to add another view for this implementation detail.
>
> The when constraint can express a software desire which can be reasonable
> to some degree. You would like to be sure that a null pointer will not occur
> after a corresponding check succeeded.

He wants to be sure that the true branch through a NULL pointer check is
not taken.

> * But I feel unsure about the circumstances under which the Coccinelle software
>   can determine this aspect actually.
>
> * I find that it can eventually make sense only after the content of
>   the local variable (which is identified by “x”) was modified.
>   Thus I would find the exclusion of assignments more useful at this place.

I assume that it was added because it was found to be useful.  Please
actually try things out before declaring them to be useless.

julia

Re: [v2] coccinelle: semantic code search for missing of_node_put

[Markus Elfring]

>>>> +if (x == NULL || ...) S
>>>> +... when != e = (T)x
>>>> +    when != true x == NULL
…
> I assume that it was added because it was found to be useful.

We can get different software development opinions also on
this implementation detail.


> Please actually try things out before declaring them to be useless.

Further clarification of desirable software behaviour will help.
I dare to express doubts around the SmPL functionality “when != true x == NULL”.

Would any more contributors like to share additional insights for the safer
application of the semantic patch language?

Is a reassignment of such local variable an usual precondition for
the discussed programming concern?

Regards,
Markus

Re: [v2] Coccinelle: Testing SmPL constraints

[Markus Elfring]

> Please actually try things out before declaring them to be useless.

This feedback provides also another opportunity for collateral evolution
in some directions. I am curious on how involved uncertainty can be fixed around
possibly different interpretation for provided software functionality.

The SmPL construct “... when …” is mentioned in an area (of the section
“Basic transformations” in the software documentation) which is introduced with
the wording “The grammar for the minus or plus slice of a transformation is
as follows:”.
https://github.com/coccinelle/coccinelle/blob/c6d7554edf7c4654aeae4d33c3f040e300682f23/docs/manual/cocci_syntax.tex#L1033

I got the impression that the corresponding meaning is not explained in this
information source so far.
The published example “Reference counter: the of_xxx API” can be interesting
then to some degree for the explanation of the discussed development efforts.
https://github.com/coccinelle/coccinelle/blob/175de16bc7e535b6a89a62b81a673b0d0cd7075c/docs/manual/examples.tex#L320

If the available application documentation is still too limited (and incomplete
because it is also work in progress), it is probably usual that SmPL code
occasionally tries to express expectations which are not covered by an evolving
software implementation.
How would you like to improve the situation further?

* Is it certain that a search is performed only for the source code “x == NULL”
  (and corresponding isomorphisms) by the SmPL constraint “when != true”
  (after a successful null pointer check was detected in this use case)?

* Would you like to test any functionality which should work in different ways
  than you might see from the original OCaml source code?
  https://github.com/coccinelle/coccinelle/issues/134

Regards,
Markus