Re: [Cocci] [v6] coccinelle: semantic code search for missing put_device()
> Do you have any other questions? I would like to point another aspect out for further development considerations. The initial assignment targets are (id)expressions in the discussed analysis approach so far. Would you like to care also for value (or pointer) initialisations by resource allocation calls at the following source code places? * Variable definition * Designated initialiser Will possible software extensions trigger interesting challenges? Regards, Markus ___ 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()
>> Will corrections become relevant for specifications in (assignment) >> exclusions >> of the second SmPL ellipsis in the discussed script? > > Let's do some experiments with the code in the current kernel. It seems that you provided additional information for the adjustment of when specifications according to the first SmPL ellipsis. Other details are still in the waiting queue for the desired software clarification. Regards, Markus ___ 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()
> > Do you have any other questions?
>
> Obviously, yes.
> I am curious if this development discussion and code review will trigger
> further software adjustments.
> I guess that you will need additional time to reconsider specific items
> from recent feedback.
>
> Will corrections become relevant for specifications in (assignment) exclusions
> of the second SmPL ellipsis in the discussed script?
Let's do some experiments with the code in the current kernel.
Let us take this code as an example:
Https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/sound/soc/fsl/imx-sgtl5000.c
1, Original code
static int imx_sgtl5000_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct device_node *ssi_np, *codec_np;
struct platform_device *ssi_pdev;
...
ssi_pdev = of_find_device_by_node(ssi_np);
...
}
Our current patch will find the problem:
./sound/soc/fsl/imx-sgtl5000.c:169:1-7: ERROR: missing put_device; call
of_find_device_by_node on line 105, but without a corresponding object release
within this function.
./sound/soc/fsl/imx-sgtl5000.c:177:1-7: ERROR: missing put_device; call
of_find_device_by_node on line 105, but without a corresponding object release
within this function.
The problem can be detected by both of the following methods:
+id = of_find_device_by_node@p1(x)
+... when != e = id
...
Or:
...
+ ... when != id = e
2, Suppose we change it to:
static int imx_sgtl5000_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
struct device_node *ssi_np, *codec_np;
struct platform_device *ssi_pdev;
...
ssi_pdev = of_find_device_by_node(ssi_np);
...
ssi_pdev = to_platform_device(XYZ);
}
But this time, only the first method can detect the problem.
The second method, although the false positive rate is lower,
but the recall rate is reduced, we may miss some real issues.
Thanks,
Regards,
Wen___
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()
> Do you have any other questions? Obviously, yes. I am curious if this development discussion and code review will trigger further software adjustments. I guess that you will need additional time to reconsider specific items from recent feedback. Will corrections become relevant for specifications in (assignment) exclusions of the second SmPL ellipsis in the discussed script? Regards, Markus ___ 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()
>> Although we have tested these two methods in the existing kernel code, >> considering the evolution of the kernel code, these special cases may occur, >> so we are willing to take them into account. >> We plan to modify the code like this: >> >> id = of_find_device_by_node@p1(x) >> -... when != e = id >> +... when != e = (T)id >> +when != id = (T)e > > This change is fine with me. Thanks for another positive feedback on such software implementation details. Regards, Markus ___ 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()
On Tue, 19 Feb 2019, wen.yan...@zte.com.cn wrote: > > > I would have a hard time saying which one is more reasonable to test, > > I suggest to reconsider the interpretation of this software situation once > > more. > > > since both are extremely unlikely. > > I disagree to this view because two ellipses were intentionally specified > > in published SmPL scripts. > > So some software developers found these “special use cases” important > > enough. > > >> In addition, we feel that we should probably accept this patch first, > > I disagree to this imagination because I would prefer to integrate a source > > code variant > > without a bug (which was copied from a version on 2013-05-08 by Petr > > Strnad). > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/pci_free_consistent.cocci?id=f7b167113753e95ae61383e234f8d10142782ace#n12 > > I hope that nicer run time behaviour can become also relevant here. > > Both cases are extremely unlikely. > Although we have tested these two methods in the existing kernel code, > considering the evolution of the kernel code, these special cases may occur, > so we are willing to take them into account. > We plan to modify the code like this: > > id = of_find_device_by_node@p1(x) > -... when != e = id > +... when != e = (T)id > +when != id = (T)e This change is fine with me. julia > > Do you have any other questions? > Thanks. > > Regards, > Wen___ 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()
>>> Which data element should not get reassigned here (before a corresponding >>> null pointer check)? >>> >> >> Thank you for your comments. >> We did some experiments: >> +id = of_find_device_by_node@p1(x) >> +... when != e = id >> ... >> Or: >> ... >> + ... when != id = e >> >> The number of issuses found by these two methods is the same. This can be because these SmPL specifications share some source code search functionality. >> When != e = id achieves this behavior. > > They are the same because neither issue arises. You might not notice a difference from a specific source file selection so far. > I would have a hard time saying which one is more reasonable to test, I suggest to reconsider the interpretation of this software situation once more. > since both are extremely unlikely. I disagree to this view because two ellipses were intentionally specified in published SmPL scripts. So some software developers found these “special use cases” important enough. >> In addition, we feel that we should probably accept this patch first, I disagree to this imagination because I would prefer to integrate a source code variant without a bug (which was copied from a version on 2013-05-08 by Petr Strnad). https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/pci_free_consistent.cocci?id=f7b167113753e95ae61383e234f8d10142782ace#n12 I hope that nicer run time behaviour can become also relevant here. >> use it to find more memory leaks, and solve the actual problems in the >> kernel code. Your are not hindered to achieve specific software improvements with evolving development approaches while the clarification and the final integration of useful scripts for the semantic patch language can take a bit longer. >> As for the patch itself, we can continue to pursue perfect in the process >> of using it to solve practical problems. I am curious on how your attention will evolve further for the corresponding software correctness. 1. How much will you care for the order of identifiers within the application of SmPL assignment exclusions? 2. Would you like to take additional data type restrictions into account? Regards, Markus ___ 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()
On Mon, 18 Feb 2019, wen.yan...@zte.com.cn wrote: > > > when != e = id achieves this behavior. > > > > I can not agree to this view completely because of the meaning that is > > connected > > with these variable identifiers. > > > > Both metavariables share the kind “expression”. So I can imagine > > that there is an intersection for the source code match possibility. > > But one was intentionally restricted to the kind “local idexpression” so > > far. > > > > Which data element should not get reassigned here (before a corresponding > > null pointer check)? > > > > Thank you for your comments. > We did some experiments: > +id = of_find_device_by_node@p1(x) > +... when != e = id > ... > Or: > ... > + ... when != id = e > > The number of issuses found by these two methods is the same. > When != e = id achieves this behavior. They are the same because neither issue arises. I would have a hard time saying which one is more reasonable to test, since both are extremely unlikely. julia > > In addition, we feel that we should probably accept this patch first, use it > to find more memory leaks, and solve the actual problems in the kernel code. > As for the patch itself, we can continue to pursue perfect in the process of > using it to solve practical problems. > > Regards, > Wen___ 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()
> > when != e = id achieves this behavior. > > I can not agree to this view completely because of the meaning that is > connected > with these variable identifiers. > > Both metavariables share the kind “expression”. So I can imagine > that there is an intersection for the source code match possibility. > But one was intentionally restricted to the kind “local idexpression” so far. > > Which data element should not get reassigned here (before a corresponding > null pointer check)? > Thank you for your comments. We did some experiments: +id = of_find_device_by_node@p1(x) +... when != e = id ... Or: ... + ... when != id = e The number of issuses found by these two methods is the same. When != e = id achieves this behavior. In addition, we feel that we should probably accept this patch first, use it to find more memory leaks, and solve the actual problems in the kernel code. As for the patch itself, we can continue to pursue perfect in the process of using it to solve practical problems. Regards, Wen___ 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()
On Sun, 17 Feb 2019, Markus Elfring wrote: > >> If you would insist on the specification of such an assignment exclusion > >> for a SmPL ellipsis: > >> Can we agree on a correct order? > > > > I don't get your point. > > I propose to take another closer look at a bit of SmPL code. > > > > There is no correct order. > > I have got an other software development view here. > > > > Each order expresses something different. > > I agree to this information. > > > > The order that is currently in the semantic patch is the one > > that is more likely in practice. > > Please check once more. > > … > +@search exists@ > +local idexpression id; > +expression x,e,e1; > +position p1,p2; > … > +@@ > + > +id = of_find_device_by_node@p1(x) > +... when != e = id > … > > Or: > > … > + ... when != id = e > … > > > Which SmPL specification will achieve the desired software behaviour? The desired behavior is to check whether the allocated value is saved in some other variable (typically a structure field) and thus it doesn't need to be freed just because the original local variable goes out of scope at the end of the function. when != e = id achieves this behavior. julia___ 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()
>> If you would insist on the specification of such an assignment exclusion >> for a SmPL ellipsis: >> Can we agree on a correct order? > > I don't get your point. I propose to take another closer look at a bit of SmPL code. > There is no correct order. I have got an other software development view here. > Each order expresses something different. I agree to this information. > The order that is currently in the semantic patch is the one > that is more likely in practice. Please check once more. … +@search exists@ +local idexpression id; +expression x,e,e1; +position p1,p2; … +@@ + +id = of_find_device_by_node@p1(x) +... when != e = id … Or: … + ... when != id = e … Which SmPL specification will achieve the desired software behaviour? Regards, Markus ___ 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()
On Sun, 17 Feb 2019, Markus Elfring wrote: > >> Would you dare to interpret my update suggestion (reordering of two > >> identifiers) > >> as a required SmPL script correction? > > > > I didn't suggest to reorder anything. > > This is obvious according to your acknowledgement for the sixth version > of this evolving SmPL script. > > > > Both are needed. > > If you would insist on the specification of such an assignment exclusion > for a SmPL ellipsis: > Can we agree on a correct order? I don't get your point. There is no correct order. Each order expresses something different. The order that is currently in the semantic patch is the one that is more likely in practice. julia > > > > And, no I don't consider it to be a required suggestion. > > Have we got a different view about an implementation detail at this place? > > > > In practice, reassigning such a variable is very unlikely. > > This can be. > > Regards, > Markus > ___ 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()
>> Would you dare to interpret my update suggestion (reordering of two >> identifiers) >> as a required SmPL script correction? > > I didn't suggest to reorder anything. This is obvious according to your acknowledgement for the sixth version of this evolving SmPL script. > Both are needed. If you would insist on the specification of such an assignment exclusion for a SmPL ellipsis: Can we agree on a correct order? > And, no I don't consider it to be a required suggestion. Have we got a different view about an implementation detail at this place? > In practice, reassigning such a variable is very unlikely. This can be. Regards, Markus ___ 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
