On Wed, 15 May 2019, [email protected] 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 <[email protected]>
> > > ---
> > > 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
> > >
> > >
>> 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