Re: [PATCH 3/3] Coccinelle: pci_free_consistent: Extend whenconstraints for two SmPL ellipses

Re: [PATCH 3/3] Coccinelle: pci_free_consistent: Extend whenconstraints for two SmPL ellipses

[Julia Lawall]

On Wed, 15 May 2019, wen.yang99@zte.com.cn wrote:

> Hello,
> > >
> > > A SmPL ellipsis was specified for a search approach so that additional
> > > source code would be tolerated between an assignment to a local variable
> > > and the corresponding null pointer check.
> > >
> > > But such code should be restricted.
> > > * The local variable must not be reassigned there.
> > > * It must also not be forwarded to an other assignment target.
> > >
> > > Take additional casts for these code exclusion specifications into account
> > > together with optional parentheses.
> >
> > I leave this up to the ZTE people.
> > julia
> >
> Thanks.
>
> 1, "id = (T2)(e)" is rare.

Thanks for checking.  I don't really care if it is rare.  There should not
be much cost to this.  On the other hand, I do care about causing false
negatives.  I don't know any more what is the type of id.  Making it
identifier would lead to false negatives as noted below.  It would be
better as something like e2->fld.

julia

> It may be a minor detail that will have no impact in practice.
> We've tested it, and this SmPL may only need to fix the following two false positives:
> ./drivers/net/ethernet/cisco/enic/vnic_dev.c:861:1-7: ERROR: missing pci_free_consistent; pci_alloc_consistent on line 855 and return without freeing on line 861
> ./drivers/message/fusion/mptctl.c:2643:2-8: ERROR: missing pci_free_consistent; pci_alloc_consistent on line 2511 and return without freeing on line 2643
>
> 2,  If you really plan to add the two restrictions above,
> you may need to consider this further than simply adding a "when != id = (T2)(e)" statement.
> I constructed the flollowing code snippet as a test case:
>
> int test()
> {
> 	unsigned char *new_page = NULL;
> 	int ret;
>
> 	new_page = pci_alloc_consistent(dev, 4096, &temp_pci);
> 	if (new_page == NULL)
>     return -1;
> X;
> 	Y;
> 	new_page = Z;
>
> 	return ret;
> }
>
> Using the original SmPL, we can find a bug.
> But with your modified SmPL, we can't find the bug.
>
> --
> Regards,
> Wen
>
> >
> >
> > >
> > > Fixes: f7b167113753e95ae61383e234f8d10142782ace ("scripts: Coccinelle script for pci_free_consistent()")
> > > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> > > ---
> > >  scripts/coccinelle/free/pci_free_consistent.cocci | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/scripts/coccinelle/free/pci_free_consistent.cocci b/scripts/coccinelle/free/pci_free_consistent.cocci
> > > index 45bc14ece151..48a36adfa3ce 100644
> > > --- a/scripts/coccinelle/free/pci_free_consistent.cocci
> > > +++ b/scripts/coccinelle/free/pci_free_consistent.cocci
> > > @@ -13,13 +13,15 @@ virtual org
> > >  local idexpression id;
> > >  expression x,y,z,e;
> > >  position p1,p2;
> > > -type T;
> > > +type T,T2,T3,T4;
> > >  @@
> > >
> > >  id = pci_alloc_consistent@p1(x,y,&z)
> > > -... when != e = id
> > > + ... when != id = (T2)(e)
> > > +     when != e = (T3)(id)
> > >  if (id == NULL || ...) { ... return ...; }
> > >  ... when != pci_free_consistent(x,y,id,z)
> > > +    when != id = (T4)(e)
> > >      when != if (id) { ... pci_free_consistent(x,y,id,z) ... }
> > >      when != if (y) { ... pci_free_consistent(x,y,id,z) ... }
> > >      when != e = (T)id
> > > --
> > > 2.21.0
> > >
> > >

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

>> 1, "id = (T2)(e)" is rare.
>
> Thanks for checking.  I don't really care if it is rare.

I got related source code analysis concerns.


> There should not be much cost to this.

Each additional filter will influence the software run times and
possible results.


> On the other hand, I do care about causing false negatives.

Do you find the missing warning after the addition of such an exclusion
specification interesting?

Regards,
Markus

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

>>> On the other hand, I do care about causing false negatives.
>>
>> Do you find the missing warning after the addition of such an exclusion
>> specification interesting?
>
> I already suggested how to improve the code.

I find that the idea “e2->fld” needs further clarification.
Such a SmPL specification will be resolved also to an expression,
won't it?

I guess that it can fit better to other analysis approaches.
Will any script variants become more interesting?

Is it still unclear which detail (besides the added SmPL filter)
will hinder the warning display for Wen's test case?

Regards,
Markus

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Julia Lawall]

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



On Wed, 15 May 2019, Markus Elfring wrote:

> >>> On the other hand, I do care about causing false negatives.
> >>
> >> Do you find the missing warning after the addition of such an exclusion
> >> specification interesting?
> >
> > I already suggested how to improve the code.
>
> I find that the idea “e2->fld” needs further clarification.
> Such a SmPL specification will be resolved also to an expression,
> won't it?

Saving in a local variable doesn't impact the need to free the object.  A
field is the most obvious case where the object may not need freeing.  But
there are many expressions that e2->fld will not match.

julia

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

Am 15.05.19 um 12:19 schrieb Julia Lawall:
>
>
> On Wed, 15 May 2019, Markus Elfring wrote:
>
>>>>> On the other hand, I do care about causing false negatives.
>>>>
>>>> Do you find the missing warning after the addition of such an exclusion
>>>> specification interesting?
>>>
>>> I already suggested how to improve the code.
>>
>> I find that the idea “e2->fld” needs further clarification.
>> Such a SmPL specification will be resolved also to an expression,
>> won't it?
>
> Saving in a local variable doesn't impact the need to free the object.

I suggest to reconsider this view.

Would we like to introduce additional case distinctions for the handling
of reassignments to local variables (as shown in Wen's test case)?


> A field is the most obvious case where the object may not need freeing.

A corresponding resource release should probably be performed by
an other function then.


> But there are many expressions that e2->fld will not match.

Data structure members can eventually belong also to a local variable.
Would they become relevant for further SmPL exclusion specifications?

Regards,
Markus

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

> 1, "id = (T2)(e)" is rare.

I can follow this view for such a filter.


> It may be a minor detail that will have no impact in practice.

I suggest to reconsider this view once more.

Should such exclusion specifications take also “unexpected” source code
into account so that analysis results will be presented with a low
false positive rate?


> We've tested it, and this SmPL may only need to fix the following two false positives:

Thanks for your acknowledgement that my proposal can give us
another useful effect.


> 2,  If you really plan to add the two restrictions above,
> you may need to consider this further than simply adding a "when != id = (T2)(e)" statement.
> I constructed the flollowing code snippet as a test case:
…
> Using the original SmPL, we can find a bug.

I observe on my system that I do not get a desired warning
by the software combination “Coccinelle 1.0.7-00186-g99e081e9 (OCaml 4.07.1)”
(even from the unmodified SmPL script) for your test example.

Which version are you using for the spatch program?


> But with your modified SmPL, we can't find the bug.

I do not see a difference here. - I wonder also about this situation then.

But this gives us the opportunity to clarify the really desired
software behaviour in more detail.
How many developers would like to help with additional insights?

Regards,
Markus

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

>> But with your modified SmPL, we can't find the bug.
>
> I do not see a difference here.

I have accidentally overlooked to pass the parameter “-D report”
to the spatch program.
Now I notice also an unexpected and undesirable difference for your test example
after the suggested addition of exclusion specifications.


> - I wonder also about this situation then.

How should the software behaviour be improved finally?

Will we need to work with any SmPL script versions in parallel
for selected source code analysis configurations?

Regards,
Markus

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

> 1, "id = (T2)(e)" is rare.

I can follow this view for such a filter.


> It may be a minor detail that will have no impact in practice.

I suggest to reconsider this view once more.

Should such exclusion specifications take also “unexpected” source code
into account so that analysis results will be presented with a low
false positive rate?


> We've tested it, and this SmPL may only need to fix the following two false positives:

Thanks for your acknowledgement that my proposal can give us
another useful effect.


> 2,  If you really plan to add the two restrictions above,
> you may need to consider this further than simply adding a "when != id = (T2)(e)" statement.
> I constructed the flollowing code snippet as a test case:
…
> Using the original SmPL, we can find a bug.

I observe on my system that I do not get a desired warning
by the software combination “Coccinelle 1.0.7-00186-g99e081e9 (OCaml 4.07.1)”
(even from the unmodified SmPL script) for your test example.

Which version are you using for the spatch program?


> But with your modified SmPL, we can't find the bug.

I do not see a difference here. - I wonder also about this situation then.

But this gives us the opportunity to clarify the really desired
software behaviour in more detail.
How many developers would like to help with additional insights?

Regards,
Markus

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

> 2,  If you really plan to add the two restrictions above,
> you may need to consider this further than simply adding a "when != id = (T2)(e)" statement.
> I constructed the flollowing code snippet as a test case:
…
> Using the original SmPL, we can find a bug.
> But with your modified SmPL, we can't find the bug.

The difference is the possible exclusion of a reassignment to a local variable
in which a pointer was stored for a resource allocation.

Would you like to adjust corresponding case distinctions any further?

How many script variants will become relevant here?

Regards,
Markus

Re: [3/3] Coccinelle: pci_free_consistent: Extend when constraints for two SmPL ellipses

[Markus Elfring]

> I don't think it is an environmental issue.

You are right about taking other parameters better into account.


> Theoretically, the original SmPL should find bugs.

I expect also that both script versions should display a warning.


> Although, I still send you my environmental information:

Thanks.


> spatch version 1.0.7 compiled with OCaml version 4.06.0

This shows that our software variants are recent enough.


> You may not take a serious look at the pci_free_consistent.cocci script,

I try to handle my update suggestions and your constructive feedback seriously.


> it needs to pass in the virtual variable report.

I did that a moment later.

I would like to apologise for a temporarily inappropriate parameter selection.
How will the clarification of the shown software behaviour evolve further?

Regards,
Markus