| Message ID | alpine.DEB.2.02.1512262156580.2070@localhost6.localdomain6 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/26/2015 11:58 PM, Julia Lawall wrote: > The error return value of platform_get_irq seems to often get dropped. > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > --- > > v2: Check for the direct return case also. Added some mailing lists of > common offenders. > > diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci b/scripts/coccinelle/api/platform_get_irq_return.cocci > new file mode 100644 > index 0000000..44680d0 > --- /dev/null > +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci > @@ -0,0 +1,58 @@ > +/// Propagate the return value of platform_get_irq. > +//# Sometimes the return value of platform_get_irq is tested using <= 0, but 0 > +//# might not be an appropriate return value in an error case. > +/// > +// Confidence: Moderate > +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2. > +// URL: http://coccinelle.lip6.fr/ > +// Options: --no-includes --include-headers > + > +virtual context > +virtual org > +virtual report > + > +// ---------------------------------------------------------------------------- > + > +@r depends on context || org || report@ > +constant C; > +statement S; > +expression e, ret; > +position j0, j1; > +@@ > + > +* e@j0 = platform_get_irq(...); > +( > +if@j1 (...) { > + ... > + return -C; > +} else S > +| > +if@j1 (...) { > + ... > + ret = -C; > + ... > + return ret; > +} else S Well, this seems to also cover the (e <= 0) checks which do make same sense in the light of Linus considering IRQ0 invalid. So I'd be more specific about the checks here -- 0 should indeed be overridden with something if it's considered invalid. MBR, Sergei
On Sun, 27 Dec 2015, Sergei Shtylyov wrote: > On 12/26/2015 11:58 PM, Julia Lawall wrote: > > > The error return value of platform_get_irq seems to often get dropped. > > > > Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> > > > > --- > > > > v2: Check for the direct return case also. Added some mailing lists of > > common offenders. > > > > diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci > > b/scripts/coccinelle/api/platform_get_irq_return.cocci > > new file mode 100644 > > index 0000000..44680d0 > > --- /dev/null > > +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci > > @@ -0,0 +1,58 @@ > > +/// Propagate the return value of platform_get_irq. > > +//# Sometimes the return value of platform_get_irq is tested using <= 0, > > but 0 > > +//# might not be an appropriate return value in an error case. > > +/// > > +// Confidence: Moderate > > +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2. > > +// URL: http://coccinelle.lip6.fr/ > > +// Options: --no-includes --include-headers > > + > > +virtual context > > +virtual org > > +virtual report > > + > > +// > > ---------------------------------------------------------------------------- > > + > > +@r depends on context || org || report@ > > +constant C; > > +statement S; > > +expression e, ret; > > +position j0, j1; > > +@@ > > + > > +* e@j0 = platform_get_irq(...); > > +( > > +if@j1 (...) { > > + ... > > + return -C; > > +} else S > > +| > > +if@j1 (...) { > > + ... > > + ret = -C; > > + ... > > + return ret; > > +} else S > > Well, this seems to also cover the (e <= 0) checks which do make same sense > in the light of Linus considering IRQ0 invalid. So I'd be more specific about > the checks here -- 0 should indeed be overridden with something if it's > considered invalid. That's what the limitations section says (lines with #). This doesn't make any changes, it only makes warnings, which should include the limitations information, so perhaps people can consider what it is that they really intend to do. If you think this is not a good idea, then I can make the test more specific. julia
On 12/27/2015 01:32 AM, Julia Lawall wrote: >>> The error return value of platform_get_irq seems to often get dropped. >>> >>> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> >>> >>> --- >>> >>> v2: Check for the direct return case also. Added some mailing lists of >>> common offenders. >>> >>> diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci >>> b/scripts/coccinelle/api/platform_get_irq_return.cocci >>> new file mode 100644 >>> index 0000000..44680d0 >>> --- /dev/null >>> +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci >>> @@ -0,0 +1,58 @@ >>> +/// Propagate the return value of platform_get_irq. >>> +//# Sometimes the return value of platform_get_irq is tested using <= 0, >>> but 0 >>> +//# might not be an appropriate return value in an error case. >>> +/// >>> +// Confidence: Moderate >>> +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2. >>> +// URL: http://coccinelle.lip6.fr/ >>> +// Options: --no-includes --include-headers >>> + >>> +virtual context >>> +virtual org >>> +virtual report >>> + >>> +// >>> ---------------------------------------------------------------------------- >>> + >>> +@r depends on context || org || report@ >>> +constant C; >>> +statement S; >>> +expression e, ret; >>> +position j0, j1; >>> +@@ >>> + >>> +* e@j0 = platform_get_irq(...); >>> +( >>> +if@j1 (...) { >>> + ... >>> + return -C; >>> +} else S >>> +| >>> +if@j1 (...) { >>> + ... >>> + ret = -C; >>> + ... >>> + return ret; >>> +} else S >> >> Well, this seems to also cover the (e <= 0) checks which do make same sense >> in the light of Linus considering IRQ0 invalid. So I'd be more specific about >> the checks here -- 0 should indeed be overridden with something if it's >> considered invalid. > > That's what the limitations section says (lines with #). This doesn't Ah, failed to notice those, only saw after replying. > make any changes, it only makes warnings, which should include the > limitations information, so perhaps people can consider what it is that > they really intend to do. > > If you think this is not a good idea, then I can make the test more > specific. Well, looking again, the patch should be good. I just thought its goal was to fix the code as well... > julia MBR, Sergei
> Well, looking again, the patch should be good. I just thought its goal was > to fix the code as well... I could do that for the irq < 0 case, but I think that in that case, kbuild will only run the patch version, and the <= cases will not be reported on. I don't have a general fix for the <= 0. Is it even correct to have < in some cases and <= in others? julia
> The error return value of platform_get_irq seems to often get dropped. How do you think about any more fine-tuning here? Commit message: * … of the platform_get_irq() function seems to get dropped too often. * Why do you concentrate on a single function name? Do you plan to extend this source code analysis approach? > +@script:python r_report depends on report@ > +j0 << r.j0; > +j1 << r.j1; > +@@ > + > +msg = "Propagate return value of platform_get_irq around line %s." % (j1[0].line) Are there more unchecked return values which are interesting for further considerations? https://cwe.mitre.org/data/definitions/252.html Regards, Markus
On 12/27/2015 9:13 AM, Julia Lawall wrote: >> Well, looking again, the patch should be good. I just thought its goal was >> to fix the code as well... > > I could do that for the irq < 0 case, but I think that in that case, kbuild > will only run the patch version, and the <= cases will not be reported on. > I don't have a general fix for the <= 0. Is it even correct to have < in > some cases and <= in others? That's a good question... In my prior fixes of this case I preferred to consider IRQ0 valid and so used 'irq < 0'. I myself don't share the "IRQ0 is invalid" sentiment... > julia MBR, Sergei
On Sun, 27 Dec 2015, SF Markus Elfring wrote: > > The error return value of platform_get_irq seems to often get dropped. > > How do you think about any more fine-tuning here? > > Commit message: > * … of the platform_get_irq() function seems to get dropped too often. > > * Why do you concentrate on a single function name? > Do you plan to extend this source code analysis approach? > > > > +@script:python r_report depends on report@ > > +j0 << r.j0; > > +j1 << r.j1; > > +@@ > > + > > +msg = "Propagate return value of platform_get_irq around line %s." % (j1[0].line) > > Are there more unchecked return values which are interesting > for further considerations? > https://cwe.mitre.org/data/definitions/252.html The value is not unchecked. I made a specific rule because the specific problem is quite common. julia
>> https://cwe.mitre.org/data/definitions/252.html > > The value is not unchecked. Would you like to express any stronger relationship between the function call example and the occurrence of an if statement by the discussed SmPL script? > I made a specific rule because the specific problem is quite common. Can it become also interesting to generalise this search pattern? Regards, Markus
diff --git a/scripts/coccinelle/api/platform_get_irq_return.cocci b/scripts/coccinelle/api/platform_get_irq_return.cocci new file mode 100644 index 0000000..44680d0 --- /dev/null +++ b/scripts/coccinelle/api/platform_get_irq_return.cocci @@ -0,0 +1,58 @@ +/// Propagate the return value of platform_get_irq. +//# Sometimes the return value of platform_get_irq is tested using <= 0, but 0 +//# might not be an appropriate return value in an error case. +/// +// Confidence: Moderate +// Copyright: (C) 2015 Julia Lawall, Inria. GPLv2. +// URL: http://coccinelle.lip6.fr/ +// Options: --no-includes --include-headers + +virtual context +virtual org +virtual report + +// ---------------------------------------------------------------------------- + +@r depends on context || org || report@ +constant C; +statement S; +expression e, ret; +position j0, j1; +@@ + +* e@j0 = platform_get_irq(...); +( +if@j1 (...) { + ... + return -C; +} else S +| +if@j1 (...) { + ... + ret = -C; + ... + return ret; +} else S +) + +// ---------------------------------------------------------------------------- + +@script:python r_org depends on org@ +j0 << r.j0; +j1 << r.j1; +@@ + +msg = "Propagate return value of platform_get_irq." +coccilib.org.print_todo(j0[0], msg) +coccilib.org.print_link(j1[0], "") + +// ---------------------------------------------------------------------------- + +@script:python r_report depends on report@ +j0 << r.j0; +j1 << r.j1; +@@ + +msg = "Propagate return value of platform_get_irq around line %s." % (j1[0].line) +coccilib.report.print_report(j0[0], msg) +
The error return value of platform_get_irq seems to often get dropped. Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr> --- v2: Check for the direct return case also. Added some mailing lists of common offenders.