Re: [Cocci] 答复: [v6] coccinelle: semantic code search for missing put_device()
> But please also refer to the examples of coccinelle, such as: > http://coccinelle.lip6.fr/rules/kmalloc.html > and > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/pci_free_consistent.cocci These scripts for the semantic patch language show some software design possibilities. They contain implementation details which can be also worth for additional development considerations. Will systematic refactoring become more interesting? > You will find that there are differences between coccinelle and c. Would you like to discuss any of them further? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] 答复: [v6] coccinelle: semantic code search for missing put_device()
Hi Markus, > > In a function, for a local variable obtained by of_find_device_by_node(), > > I got a software understanding where such a variable can not be obtained > from this function call. > The return value (like a pointer in this use case) can be stored there. > > > v6: > > - to be double sure, replace >dev with (T)(>dev). > > The support for data type casts is another interesting extension for > this source code analysis approach. > Further adjustments might become possible at other places of the presented > SmPL script > after specific clarifications of previously mentioned implementation details. First of all, thank you for your comments. But please also refer to the examples of coccinelle, such as: http://coccinelle.lip6.fr/rules/kmalloc.html and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/pci_free_consistent.cocci You will find that there are differences between coccinelle and c. Regards, Wen ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Qualifiers for field types get lost
On Sat, 16 Feb 2019, Michael Stefaniuc wrote: > On 2/16/19 8:20 AM, Julia Lawall wrote: > > I have fixed the array issue. > Thanks! Tested it and it looks good. Great! Thanks for the report. The improvement could be useful for other type checking things. julia ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Qualifiers for field types get lost
On 2/16/19 8:20 AM, Julia Lawall wrote: > I have fixed the array issue. Thanks! Tested it and it looks good. bye michael ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Qualifiers for field types get lost
On 2/16/19 7:00 AM, Markus Elfring wrote: >> The generic transformation is simple but perfect at the same time and >> showcases coccinelles expressiveness and beauty. > > Another “false positive leftover” pointed further software development > opportunities out according to your suggestion “[PATCH] oleaut32/tests: > Propagate the const instead of casting it away”, didn't it? > https://source.winehq.org/patches/data/158631 ??? a.) That patch is not a cast to self so no "further software development opportunities" to my selfcast.cocci script. b.) Actually doing code cleanup is a better way of finding new "opportunities" for more cleanup. The vague hints for "further software development opportunities" are actually harmful as they add noise and detract from the bug report. bye michael ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] Qualifiers for field types get lost
On 2/16/19 7:33 AM, Markus Elfring wrote: >> There's a qualifier propagation issue left in the case the field is an array. >> I have attached a patch that extends the tests/qualifier.c test case >> to demonstrate the problem. > > Can the desired functionality check be performed also with a bit less > source code by using the conditional operator? Sure it can. Would it be better? No. Exercise left to the interested reader why I consider my variant to be better. > > -return (int *)>i; > +return b->i ? (int *)>i : (int *)>j[0]; > > > Will both test approaches be eventually needed? bye michael ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v6] coccinelle: semantic code search for missing put_device()
> In a function, for a local variable obtained by of_find_device_by_node(), I got a software understanding where such a variable can not be obtained from this function call. The return value (like a pointer in this use case) can be stored there. > v6: > - to be double sure, replace >dev with (T)(>dev). The support for data type casts is another interesting extension for this source code analysis approach. Further adjustments might become possible at other places of the presented SmPL script after specific clarifications of previously mentioned implementation details. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH v6] coccinelle: semantic code search for missing put_device()
On Sat, 16 Feb 2019, Wen Yang wrote:
> The of_find_device_by_node() takes a reference to the underlying device
> structure, we should release that reference.
> The implementation of this semantic code search is:
> In a function, for a local variable obtained by of_find_device_by_node(),
> a, if it is released by a function such as
>put_device()/of_dev_put()/platform_device_put() after the last use,
>it is considered that there is no reference leak;
> b, if it is passed back to the caller via
>dev_get_drvdata()/platform_get_drvdata()/get_device(), etc., the
>reference will be released in other functions, and the current function
>also considers that there is no reference leak;
> c, for the rest of the situation, the current function should release the
>reference by calling put_device, this code search will report an error
>with a specific confidence.
>
> By using this semantic code search, we have found some issues, such as:
> commit 11907e9d3533 ("ASoC: fsl-asoc-card: fix object reference leaks in
> fsl_asoc_card_probe")
> commit a12085d13997 ("mtd: rawnand: atmel: fix possible object reference
> leak")
> commit 11493f26856a ("mtd: rawnand: jz4780: fix possible object reference
> leak")
>
> There are still dozens of reference leaks in the current kernel code.
>
> Further, for the case of b, the object returned to other functions may also
> have a reference leak, we will continue to develop other cocci scripts to
> further check the reference leak.
>
> Signed-off-by: Wen Yang
> Reviewed-by: Julia Lawall
Acked-by: Julia Lawall
> Reviewed-by: Markus Elfring
> Cc: Julia Lawall
> Cc: Gilles Muller
> Cc: Nicolas Palix
> Cc: Michal Marek
> Cc: Markus Elfring
> Cc: Masahiro Yamada
> Cc: Wen Yang
> Cc: cheng.shen...@zte.com.cn
> Cc: cocci@systeme.lip6.fr
> Cc: linux-ker...@vger.kernel.org
> ---
> v6:
> - to be double sure, replace >dev with (T)(>dev).
> - long string literals can be accepted because of error message search
> concerns around a tool like grep
> v5:
> - exchange the word patch by code search.
> - add a SPDX identifier.
> - a split string literal can be unwanted.
> - Change the content of the reported information.
> v4:
> - add Masahiro Yamada
> - omit a blank line
> - split the long message parameter
> - reduce the number of metavariables
> - Describe the implementation of the semantic patch,
> explain the scenarios it can detect,
> and further software development considerations.
> v3:
> - reduction of a bit of redundant C code within SmPL search specifications.
> - consider the message construction without using the extra Python variable
> msg.
> v2:
> - put exists after search, and then drop the when exists below.
> - should not use the same e as in the when's below.
> - Make a new type metavariable and use it to put a cast on the result of
> platform_get_drvdata.
>
> scripts/coccinelle/free/put_device.cocci | 55
> 1 file changed, 55 insertions(+)
> create mode 100644 scripts/coccinelle/free/put_device.cocci
>
> diff --git a/scripts/coccinelle/free/put_device.cocci
> b/scripts/coccinelle/free/put_device.cocci
> new file mode 100644
> index ..96e2508c0be1
> --- /dev/null
> +++ b/scripts/coccinelle/free/put_device.cocci
> @@ -0,0 +1,55 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/// Find missing put_device for every of_find_device_by_node.
> +///
> +// Confidence: Moderate
> +// Copyright: (C) 2018-2019 Wen Yang, ZTE.
> +// Comments:
> +// Options: --no-includes --include-headers
> +
> +virtual report
> +virtual org
> +
> +@search exists@
> +local idexpression id;
> +expression x,e,e1;
> +position p1,p2;
> +type T,T1,T2;
> +@@
> +
> +id = of_find_device_by_node@p1(x)
> +... when != e = id
> +if (id == NULL || ...) { ... return ...; }
> +... when != put_device(>dev)
> +when != platform_device_put(id)
> +when != of_dev_put(id)
> +when != if (id) { ... put_device(>dev) ... }
> +when != e1 = (T)id
> +when != e1 = (T)(>dev)
> +when != e1 = get_device(>dev)
> +when != e1 = (T)platform_get_drvdata(id)
> +(
> + return
> +(id
> +|(T1)dev_get_drvdata(>dev)
> +|(T2)platform_get_drvdata(id)
> +);
> +| return@p2 ...;
> +)
> +
> +@script:python depends on report@
> +p1 << search.p1;
> +p2 << search.p2;
> +@@
> +
> +coccilib.report.print_report(p2[0],
> + "ERROR: missing put_device; call
> of_find_device_by_node on line "
> + + p1[0].line
> + + ", but without a corresponding object release
> within this function.")
> +
> +@script:python depends on org@
> +p1 << search.p1;
> +p2 << search.p2;
> +@@
> +
> +cocci.print_main("of_find_device_by_node", p1)
> +cocci.print_secs("needed put_device", p2)
> --
> 2.20.1
>
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH v6] coccinelle: semantic code search for missing put_device()
The of_find_device_by_node() takes a reference to the underlying device
structure, we should release that reference.
The implementation of this semantic code search is:
In a function, for a local variable obtained by of_find_device_by_node(),
a, if it is released by a function such as
put_device()/of_dev_put()/platform_device_put() after the last use,
it is considered that there is no reference leak;
b, if it is passed back to the caller via
dev_get_drvdata()/platform_get_drvdata()/get_device(), etc., the
reference will be released in other functions, and the current function
also considers that there is no reference leak;
c, for the rest of the situation, the current function should release the
reference by calling put_device, this code search will report an error
with a specific confidence.
By using this semantic code search, we have found some issues, such as:
commit 11907e9d3533 ("ASoC: fsl-asoc-card: fix object reference leaks in
fsl_asoc_card_probe")
commit a12085d13997 ("mtd: rawnand: atmel: fix possible object reference
leak")
commit 11493f26856a ("mtd: rawnand: jz4780: fix possible object reference
leak")
There are still dozens of reference leaks in the current kernel code.
Further, for the case of b, the object returned to other functions may also
have a reference leak, we will continue to develop other cocci scripts to
further check the reference leak.
Signed-off-by: Wen Yang
Reviewed-by: Julia Lawall
Reviewed-by: Markus Elfring
Cc: Julia Lawall
Cc: Gilles Muller
Cc: Nicolas Palix
Cc: Michal Marek
Cc: Markus Elfring
Cc: Masahiro Yamada
Cc: Wen Yang
Cc: cheng.shen...@zte.com.cn
Cc: cocci@systeme.lip6.fr
Cc: linux-ker...@vger.kernel.org
---
v6:
- to be double sure, replace >dev with (T)(>dev).
- long string literals can be accepted because of error message search concerns
around a tool like grep
v5:
- exchange the word patch by code search.
- add a SPDX identifier.
- a split string literal can be unwanted.
- Change the content of the reported information.
v4:
- add Masahiro Yamada
- omit a blank line
- split the long message parameter
- reduce the number of metavariables
- Describe the implementation of the semantic patch,
explain the scenarios it can detect,
and further software development considerations.
v3:
- reduction of a bit of redundant C code within SmPL search specifications.
- consider the message construction without using the extra Python variable msg.
v2:
- put exists after search, and then drop the when exists below.
- should not use the same e as in the when's below.
- Make a new type metavariable and use it to put a cast on the result of
platform_get_drvdata.
scripts/coccinelle/free/put_device.cocci | 55
1 file changed, 55 insertions(+)
create mode 100644 scripts/coccinelle/free/put_device.cocci
diff --git a/scripts/coccinelle/free/put_device.cocci
b/scripts/coccinelle/free/put_device.cocci
new file mode 100644
index ..96e2508c0be1
--- /dev/null
+++ b/scripts/coccinelle/free/put_device.cocci
@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: GPL-2.0
+/// Find missing put_device for every of_find_device_by_node.
+///
+// Confidence: Moderate
+// Copyright: (C) 2018-2019 Wen Yang, ZTE.
+// Comments:
+// Options: --no-includes --include-headers
+
+virtual report
+virtual org
+
+@search exists@
+local idexpression id;
+expression x,e,e1;
+position p1,p2;
+type T,T1,T2;
+@@
+
+id = of_find_device_by_node@p1(x)
+... when != e = id
+if (id == NULL || ...) { ... return ...; }
+... when != put_device(>dev)
+when != platform_device_put(id)
+when != of_dev_put(id)
+when != if (id) { ... put_device(>dev) ... }
+when != e1 = (T)id
+when != e1 = (T)(>dev)
+when != e1 = get_device(>dev)
+when != e1 = (T)platform_get_drvdata(id)
+(
+ return
+(id
+|(T1)dev_get_drvdata(>dev)
+|(T2)platform_get_drvdata(id)
+);
+| return@p2 ...;
+)
+
+@script:python depends on report@
+p1 << search.p1;
+p2 << search.p2;
+@@
+
+coccilib.report.print_report(p2[0],
+"ERROR: missing put_device; call
of_find_device_by_node on line "
+ + p1[0].line
+ + ", but without a corresponding object release
within this function.")
+
+@script:python depends on org@
+p1 << search.p1;
+p2 << search.p2;
+@@
+
+cocci.print_main("of_find_device_by_node", p1)
+cocci.print_secs("needed put_device", p2)
--
2.20.1
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] coccinelle: semantic code search for missingput_device()
>>> We don't need perfection. >> >> I guess that you noticed in the meantime that I dare to propose >> more software development efforts in such a direction. > > Yes, this is noticable. I am curious then if remaining change suggestions will be picked up by more software developers and reviewers. > It is your choice, however, other people may have their reasons > for other choices... Yes, of course. >>> We need more to eliminate the memory leaks. > > ... like this one. > >> Will this view evolve into further helpful and constructive clarifications? > > Given my above, what is the evaluation of the same question to yourself? * I hope that my contributions can improve the situation also for this software area. * Existing development tools will evolve further as usual. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] coccinelle: semantic code search for missingput_device()
Hi, On Sat, Feb 16, 2019 at 09:57:54AM +0100, Markus Elfring wrote: > > We don't need perfection. > > I guess that you noticed in the meantime that I dare to propose > more software development efforts in such a direction. Yes, this is noticable. It is your choice, however, other people may have their reasons for other choices... > > We need more to eliminate the memory leaks. ... like this one. > Will this view evolve into further helpful and constructive clarifications? Given my above, what is the evaluation of the same question to yourself? Regards, Wolfram signature.asc Description: PGP signature ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] coccinelle: semantic code search for missingput_device()
> We don't need perfection. I guess that you noticed in the meantime that I dare to propose more software development efforts in such a direction. > We need more to eliminate the memory leaks. Will this view evolve into further helpful and constructive clarifications? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] Coccinelle: semantic code search for missing put_device()
>> Thus I do not see a need (or requirement) for a duplicate search attempt.
>
> Why don't you actually try it and see what the difference is rather than
> repeatedly giving false information?
I suggest to clarify this software development disagreement by the following
SmPL code.
... when != put_stuff(my_pointer)
when != if (...) { ... put_stuff(my_pointer) ... }
Is this a specification for a bit of duplicate code?
Or:
Do you insist to find such a function call only within a branch of an if
statement?
Regards,
Markus
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] coccinelle: semantic code search for missingput_device()
On Sat, 16 Feb 2019, wen.yan...@zte.com.cn wrote: > > > Thanks, We will change it to something like this: > > > In a function, for a local variable obtained by of_find_device_by_node() > > > > How do you think about another wording approach? > > > > 1. Precondition: > > It will be checked where the return value is stored from > > a call of the function “of_find_device_by_node”. > > > > 2. The source code search will be continued with … > > Thanks. > This is more rigorous, we will follow your advice > > > > Thank you, but a local variable is necessary. > > > > Would you like to take additional storage possibilities for a safer > > analysis approach into account? > > > > Is the restriction “local” really sufficient when such a pointer > > could be copied to other variables? > > We may be able to handle this situation: > +id = of_find_device_by_node@p1(x) > ... > +when != e1 = (T)id > +when != e1 = >dev > +when != e1 = get_device(>dev) This looks good. To be double sure, you can put (T)(>dev) in the second case. When you have a chance please send the revised version. As long as I don't see that it is giving many false positives, I will accept it. We don't need perfection. We need more to eliminate the memory leaks. julia > > > > But it's over 80 characters. > > > > Long string literals can be accepted because of error message search > > concerns > > around a tool like “grep”. > > Thanks. > We will follow your advice > > >> Will any more advanced error diagnostics be eventually developed? > > > > > > Hello, we are just doing the practical work in this field. > > > > Are you aware of additional software design options from computer science > > and existing analysis tools? > > We also use the commercial software klockwork, which will also find errors in > the code, > but a lot of false positives. > > Regards, > Wen___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] coccinelle: semantic code search for missingput_device()
On Sat, 16 Feb 2019, Markus Elfring wrote: > > Thanks, We will change it to something like this: > > In a function, for a local variable obtained by of_find_device_by_node() > > How do you think about another wording approach? > > 1. Precondition: >It will be checked where the return value is stored from >a call of the function “of_find_device_by_node”. > > 2. The source code search will be continued with … > > > > Thank you, but a local variable is necessary. > > Would you like to take additional storage possibilities for a safer > analysis approach into account? > > Is the restriction “local” really sufficient when such a pointer > could be copied to other variables? I've lost track of the original semantic patch, but perhaps it needs a when != e1 = e julia > > > >> Can it happen that on other function will perform the desired reference > >> release? > > > > Thanks. > > Because the information of this local variable is not passed to the > > external function, > > this situation does not exist. > > Will copied pointers matter here? > > > > But it's over 80 characters. > > Long string literals can be accepted because of error message search concerns > around a tool like “grep”. > > > >> Will any more advanced error diagnostics be eventually developed? > > > > Hello, we are just doing the practical work in this field. > > Are you aware of additional software design options from computer science > and existing analysis tools? > > > > We also hope that it can support cross-function/cross-file/data stream > > analysis > > and other functions. > > This functionality will need further clarification. > > > > We are also analyzing the principle and code implementation of coccinelle, > > hoping to contribute a little. > > I am curious on how this situation will evolve further. > > Regards, > Markus >___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] Coccinelle: semantic code search for missing put_device()
On Sat, 16 Feb 2019, Markus Elfring wrote:
> >> We will modify the the if in the when code like this:
> >>
> >> @@ -22,7 +22,7 @@ if (id == NULL || ...) { ... return ...; }
> >> ... when != put_device(>dev)
> …
> >> -when != if (id) { ... put_device(>dev) ... }
> >> +when != if (...) { ... put_device(>dev) ... }
> >
> > This looks ok.
>
> I have got another different software development opinion also for
> such SmPL code in the adjusted line.
> I find this specific source code search variant irrelevant
> because the shown reference release function should be found
> by the first SmPL when specification already.
> Would you like to determine generally if the desired function call
> is present at all?
>
> Thus I do not see a need (or requirement) for a duplicate search attempt.
Why don't you actually try it and see what the difference is rather than
repeatedly giving false information?
julia___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] Coccinelle: semantic code search for missing put_device()
>> We will modify the the if in the when code like this:
>>
>> @@ -22,7 +22,7 @@ if (id == NULL || ...) { ... return ...; }
>> ... when != put_device(>dev)
…
>> -when != if (id) { ... put_device(>dev) ... }
>> +when != if (...) { ... put_device(>dev) ... }
>
> This looks ok.
I have got another different software development opinion also for
such SmPL code in the adjusted line.
I find this specific source code search variant irrelevant
because the shown reference release function should be found
by the first SmPL when specification already.
Would you like to determine generally if the desired function call
is present at all?
Thus I do not see a need (or requirement) for a duplicate search attempt.
Regards,
Markus
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [v5] coccinelle: semantic code search for missingput_device()
> Thanks, We will change it to something like this: > In a function, for a local variable obtained by of_find_device_by_node() How do you think about another wording approach? 1. Precondition: It will be checked where the return value is stored from a call of the function “of_find_device_by_node”. 2. The source code search will be continued with … > Thank you, but a local variable is necessary. Would you like to take additional storage possibilities for a safer analysis approach into account? Is the restriction “local” really sufficient when such a pointer could be copied to other variables? >> Can it happen that on other function will perform the desired reference >> release? > > Thanks. > Because the information of this local variable is not passed to the external > function, > this situation does not exist. Will copied pointers matter here? > But it's over 80 characters. Long string literals can be accepted because of error message search concerns around a tool like “grep”. >> Will any more advanced error diagnostics be eventually developed? > > Hello, we are just doing the practical work in this field. Are you aware of additional software design options from computer science and existing analysis tools? > We also hope that it can support cross-function/cross-file/data stream > analysis > and other functions. This functionality will need further clarification. > We are also analyzing the principle and code implementation of coccinelle, > hoping to contribute a little. I am curious on how this situation will evolve further. Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
