Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1232092 > unrolled thread

[PATCH] coccinelle: assign signed result to unsigned variable

Back to article view | Back to linux.kernel

Contents

  [PATCH] coccinelle: assign signed result to unsigned variable Andrzej Hajda <a.hajda@samsung.com> - 2015-09-24 15:00 +0200
    Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-24 18:00 +0200
      Re: [PATCH] coccinelle: assign signed result to unsigned variable Andrzej Hajda <a.hajda@samsung.com> - 2015-09-25 12:10 +0200
        Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-25 18:00 +0200
        Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-26 09:50 +0200
          Re: [PATCH] coccinelle: assign signed result to unsigned variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-26 11:10 +0200
            Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-26 11:50 +0200
              Re: [PATCH] coccinelle: assign signed result to unsigned variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-26 11:50 +0200
                Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-26 12:00 +0200
                  Re: [PATCH] coccinelle: assign signed result to unsigned variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-26 12:00 +0200
                    Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-26 13:50 +0200
                      Re: [PATCH] coccinelle: assign signed result to unsigned variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-26 16:00 +0200
                        Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-26 17:30 +0200
                          Re: [PATCH] coccinelle: assign signed result to unsigned variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-26 17:40 +0200
                            Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-26 18:00 +0200
                              Re: [PATCH] coccinelle: assign signed result to unsigned variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-26 18:00 +0200
                                Re: [PATCH] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-26 18:10 +0200
            [PATCH v2] coccinelle: assign signed result to unsigned variable Andrzej Hajda <a.hajda@samsung.com> - 2015-09-28 13:00 +0200
              Re: [PATCH v2] coccinelle: assign signed result to unsigned  variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-28 13:40 +0200
                Re: [PATCH v2] coccinelle: assign signed result to unsigned variable Andrzej Hajda <a.hajda@samsung.com> - 2015-09-28 14:10 +0200
                  Re: [PATCH v2] coccinelle: assign signed result to unsigned  variable Julia Lawall <julia.lawall@lip6.fr> - 2015-10-01 00:00 +0200
              Re: [PATCH v2] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-28 14:10 +0200
                Re: [PATCH v2] coccinelle: assign signed result to unsigned variable Andrzej Hajda <a.hajda@samsung.com> - 2015-09-28 14:20 +0200
                  Re: [PATCH v2] coccinelle: assign signed result to unsigned variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-28 14:30 +0200
                    Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned  variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-28 14:50 +0200
                      Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned  variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-28 15:00 +0200
                        Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned  variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-28 15:20 +0200
                          Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned  variable SF Markus Elfring <elfring@users.sourceforge.net> - 2015-09-28 16:00 +0200
                            Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned  variable Julia Lawall <julia.lawall@lip6.fr> - 2015-09-28 17:10 +0200
              Re: [PATCH v2] coccinelle: assign signed result to unsigned  variable Julia Lawall <julia.lawall@lip6.fr> - 2015-10-03 09:10 +0200

Page 1 of 2  [1] 2  Next page →

#1232092 — [PATCH] coccinelle: assign signed result to unsigned variable

Assigning signed function result to unsigned variable can indicate error.
To decrease number of false positives patch looks if after assignment
there is also check for negative values of the result.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi,

This patch tries to catch bugs related to losing possible negative function
results and complements my previous patch[1]. I have found about 20 real bugs
thanks to it. I will post related kernel patches on LKML.

[1]: http://permalink.gmane.org/gmane.linux.kernel/2039591
---
 .../tests/assign_signed_to_unsigned.cocci          | 48 ++++++++++++++++++++++
 1 file changed, 48 insertions(+)
 create mode 100644 scripts/coccinelle/tests/assign_signed_to_unsigned.cocci

diff --git a/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
new file mode 100644
index 0000000..ebd3d3a
--- /dev/null
+++ b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
@@ -0,0 +1,48 @@
+/// Assigning signed function result to unsigned variable can indicate error.
+/// To decrease number of false positives patch looks if after assignment
+/// there is also check for negative values of the result.
+///
+// Confidence: High
+// Copyright: (C) 2015 Andrzej Hajda, Samsung Electronics Co., Ltd. GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Options: --include-headers --all-includes
+
+virtual context
+virtual org
+virtual report
+
+@rs@
+position p;
+typedef bool, u8, u16, u32, u64, s8, s16, s32, s64;
+{char, short int, int, long, long long, s8, s16, s32, s64} vs;
+{unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long, size_t, bool, u8, u16, u32, u64} vu;
+@@
+
+vu@p = vs
+
+@r@
+position rs.p;
+identifier v, f;
+statement S1, S2;
+expression e;
+@@
+
+*v@p = f(...);
+... when != v = e;
+if ( \( v < 0 \| v <= 0 \) ) S1 else S2
+
+@script:python depends on r && org@
+p << rs.p;
+@@
+
+msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (v, f)
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on r && report@
+p << rs.p;
+f << r.f;
+v << r.v;
+@@
+
+msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (v, f)
+coccilib.report.print_report(p[0], msg)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1232214

> +@rs@
> +position p;
> +typedef bool, u8, u16, u32, u64, s8, s16, s32, s64;
> +{char, short int, int, long, long long, s8, s16, s32, s64} vs;

Can it matter to specify also the type modifier "signed" in this SmPL approach?
http://coccinelle.lip6.fr/docs/main_grammar005.html#ctype_qualif


> +{unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long, size_t, bool, u8, u16, u32, u64} vu;

How do you think about to reformat such a data type enumeration?


> +@@
> +
> +vu@p = vs
> +
> +@r@
> +position rs.p;
> +identifier v, f;
> +statement S1, S2;
> +expression e;
> +@@
> +
> +*v@p = f(...);

Do you try to check here if the value receiver is at the same source code
position from the SmPL rule "rs"?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232700

On 09/24/2015 05:51 PM, SF Markus Elfring wrote:
>> +@rs@
>> +position p;
>> +typedef bool, u8, u16, u32, u64, s8, s16, s32, s64;
>> +{char, short int, int, long, long long, s8, s16, s32, s64} vs;
> Can it matter to specify also the type modifier "signed" in this SmPL approach?
> http://coccinelle.lip6.fr/docs/main_grammar005.html#ctype_qualif

According to my tests it does not matter.
Btw I should replace short int, with short, to allow catch short intergers.

>
>
>> +{unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long, size_t, bool, u8, u16, u32, u64} vu;
> How do you think about to reformat such a data type enumeration?

If you mean split line to be below 80 chars, OK.

>
>
>> +@@
>> +
>> +vu@p = vs
>> +
>> +@r@
>> +position rs.p;
>> +identifier v, f;
>> +statement S1, S2;
>> +expression e;
>> +@@
>> +
>> +*v@p = f(...);
> Do you try to check here if the value receiver is at the same source code
> position from the SmPL rule "rs"?

Yes.
Generally I want to catch all assignments of signed function result to unsigned var.
In this script I have implemented it this way:
1. Look for all assignments 'unsigned = signed' (rs rule).
2. Check if signed from rs rule looks as a function call.

Is there better way to do it?

Regards
Andrzej

>
> Regards,
> Markus
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1232900

>>> +@rs@
>>> +position p;
>>> +typedef bool, u8, u16, u32, u64, s8, s16, s32, s64;
>>> +{char, short int, int, long, long long, s8, s16, s32, s64} vs;
>> Can it matter to specify also the type modifier "signed" in this SmPL approach?
>> http://coccinelle.lip6.fr/docs/main_grammar005.html#ctype_qualif
> According to my tests it does not matter.
> Btw I should replace short int, with short,

I have got an other view on such an implementation detail around
explicit SmPL specifications.


> to allow catch short intergers.

Do you assume that the Coccinelle software will handle more data type
variants for you automatically?


>>> +@@
>>> +
>>> +vu@p = vs
>>> +
>>> +@r@
>>> +position rs.p;
>>> +identifier v, f;
>>> +statement S1, S2;
>>> +expression e;
>>> +@@
>>> +
>>> +*v@p = f(...);
>> Do you try to check here if the value receiver is at the same source code
>> position from the SmPL rule "rs"?
> Yes.

I imagine that there is an open issue in this SmPL approach then.
How should a return value from a function call and a variable access
work at the same place?


> Is there better way to do it?

Do you need to distinguish source code positions a bit more with
corresponding SmPL variables?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233163

> Generally I want to catch all assignments of signed function result to unsigned var.

Such a static source code analysis will be useful to some degree.


> In this script I have implemented it this way:
> 1. Look for all assignments 'unsigned = signed' (rs rule).
> 2. Check if signed from rs rule looks as a function call.

I recommend to reconsider a few implementation details because I have got
the impression that this check sequence is inappropriate.


> Is there better way to do it?

I suggest to fix expression weaknesses and a design mistake in this SmPL approach.

I guess that you want to determine functions with a signed return type first
before corresponding variable assignments will be checked further.
* Would you like to collect function names for this purpose by a general analysis
  of more source files?
  (How do you think about to store them in a dedicated database?)

* Which couple of function calls will be interesting for you?

* Should the search approach take also recursively included files into account?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233173

To collect function calls that have a return value of a given type t, it
should be sufficient to do the following:

@@
t e;
identifier f;
@@

f(...)@e

The @e notation reaches upwards to match the innermost enclosing term of
the right kind (here expression).

t can of course be arbitrarily complicated.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233179

> To collect function calls that have a return value of a given type t,
> it should be sufficient to do the following:
> 
> @@
> t e;
> identifier f;
> @@
> 
> f(...)@e

Is such a SmPL approach better than a variant like the following?

@find_function@
type t;
identifier f;
@@
 t f(...)
 { ... }

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233182

On Sat, 26 Sep 2015, SF Markus Elfring wrote:

> > To collect function calls that have a return value of a given type t,
> > it should be sufficient to do the following:
> >
> > @@
> > t e;
> > identifier f;
> > @@
> >
> > f(...)@e
>
> Is such a SmPL approach better than a variant like the following?
>
> @find_function@
> type t;
> identifier f;
> @@
>  t f(...)
>  { ... }

Your approach finds a function definition.  My approach works on the call
directly, using whatever type information is available.

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233183

> Your approach finds a function definition.

Yes. - I assumed that it might also be relevant.



> My approach works on the call directly, using whatever type information is available.

The connection between the SmPL specification "f(...)@e" and the desired return type
was not obvious for me so far.

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233185

On Sat, 26 Sep 2015, SF Markus Elfring wrote:

> > Your approach finds a function definition.
>
> Yes. - I assumed that it might also be relevant.
>
>
>
> > My approach works on the call directly, using whatever type information is available.
>
> The connection between the SmPL specification "f(...)@e" and the desired return type
> was not obvious for me so far.

The nearest enclosing expression of the ) is the whole function call
itself.  e will thus match the entire expression.  e is declared to have
type t (where t is in practice signed int or whatever one wants to check
for).

julia
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233193

>> The connection between the SmPL specification "f(...)@e" and the desired return type
>> was not obvious for me so far.
> 
> The nearest enclosing expression of the ) is the whole function call itself.

Thanks for your explanation.

Now I guess that the enclosing context is a particular function implementation
where specific calls are performed, isn't it?


> e will thus match the entire expression.  e is declared to have type t

Did you omit this detail in your suggestion a moment ago?


> (where t is in practice signed int or whatever one wants to check for).

How do you think about reuse another data type enumeration there?


How would you like to manage names for functions which are not defined
in the current source file?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233206

On Sat, 26 Sep 2015, SF Markus Elfring wrote:

> >> The connection between the SmPL specification "f(...)@e" and the desired return type
> >> was not obvious for me so far.
> >
> > The nearest enclosing expression of the ) is the whole function call itself.
>
> Thanks for your explanation.
>
> Now I guess that the enclosing context is a particular function implementation
> where specific calls are performed, isn't it?

No idea what yu mean by this.  Function calls are usually found within
function definitions.  But it could be in the definition of a macro as
well.  It doesn't matter, as long as the type is available.

>
>
> > e will thus match the entire expression.  e is declared to have type t
>
> Did you omit this detail in your suggestion a moment ago?

I don't thik so.  I said t e; where t could be whatever typep or set of
types one wants.

>
> > (where t is in practice signed int or whatever one wants to check for).
>
> How do you think about reuse another data type enumeration there?

No idea what you mean by this.

>
> How would you like to manage names for functions which are not defined
> in the current source file?

Why does it matter in this case?

julia

> Regards,
> Markus
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233214

> It doesn't matter, as long as the type is available.

I suggest to make the circumstances better known when this will be the case.


>> How do you think about reuse another data type enumeration there?
> 
> No idea what you mean by this.

A SmPL variable can also be connected with a data type list which is
discussed here.


>> How would you like to manage names for functions which are not defined
>> in the current source file?
> 
> Why does it matter in this case?

* Will a command-line parameter like "--include-headers-for-types"
  be needed here?

* Would it make sense to work with function name lists in SmPL constraints?


Will any fine-tuning be needed for the execution speed of the evolving
source code analysis?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233216

On Sat, 26 Sep 2015, SF Markus Elfring wrote:

> > It doesn't matter, as long as the type is available.
>
> I suggest to make the circumstances better known when this will be the case.

It is like for the type of anything.  If the declaration of the thing is
available with the type information, eg in the same file or an included
header file, then the type will be available.  If the declaration is not
available then the type will not be available.

> >> How do you think about reuse another data type enumeration there?
> >
> > No idea what you mean by this.
>
> A SmPL variable can also be connected with a data type list which is
> discussed here.

One type, more that one type, it doesn't matter.

> >> How would you like to manage names for functions which are not defined
> >> in the current source file?
> >
> > Why does it matter in this case?
>
> * Will a command-line parameter like "--include-headers-for-types"
>   be needed here?

This argument is never needed.  It is only an optimization.  It means that
he header files are only considered when collecting type information, but
not whn doing transformation.  But this argument has no effect on the set
of types tha are available.

julia

> * Would it make sense to work with function name lists in SmPL constraints?
>
>
> Will any fine-tuning be needed for the execution speed of the evolving
> source code analysis?
>
> Regards,
> Markus
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233218

>> * Will a command-line parameter like "--include-headers-for-types"
>>   be needed here?
> 
> This argument is never needed.  It is only an optimization.  It means that
> he header files are only considered when collecting type information, but
> not whn doing transformation.  But this argument has no effect on the set
> of types tha are available.

I would consider the reuse of the parameter "--recursive-includes" then
so that the most function signatures will be available.
This has got some consequences on the execution speed and configuration
for the source code analysis.

Are there any risks to include too many functions?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233219

On Sat, 26 Sep 2015, SF Markus Elfring wrote:

> >> * Will a command-line parameter like "--include-headers-for-types"
> >>   be needed here?
> >
> > This argument is never needed.  It is only an optimization.  It means that
> > he header files are only considered when collecting type information, but
> > not whn doing transformation.  But this argument has no effect on the set
> > of types tha are available.
>
> I would consider the reuse of the parameter "--recursive-includes" then
> so that the most function signatures will be available.
> This has got some consequences on the execution speed and configuration
> for the source code analysis.
>
> Are there any risks to include too many functions?

Maybe if there are conflicting definitions of the function with different
return types.  This is probably not a big deal in practice.

julia


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1233220

>> Are there any risks to include too many functions?
> 
> Maybe if there are conflicting definitions of the function with different
> return types.  This is probably not a big deal in practice.

Are there any more concerns around the handling of conditional source code
analysis for Linux subsystems?

Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234048 — [PATCH v2] coccinelle: assign signed result to unsigned variable

Assigning signed function result to unsigned variable can indicate error.
To decrease number of false positives patch looks if after assignment
there is also check for negative values of the result.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi Julia,

Thanks for the hint. Now it looks much better.
Summarizing this patch has found 20 problems and has 22 false positives [1][2].
unsigned_lesser_than_zero.cocci patch posted earlier has found
40 problems [3][4], and about 80 false positives if I remember correctly.
Few patches were rejected, as developers likes code for testing variable range,
even if its result is always true/false [5][6], but most of kernel patches are
real bug fixes.

Both patches tries to address similar issues, maybe it would be good to merge
them? Especially as their results overlap.
Additionally I thought about adding detecting range checks in
unsigned_lesser_than_zero.cocci, to decrease number of false positives.
Of course it could then miss real bugs. What do you think about it?

[1]: http://permalink.gmane.org/gmane.linux.kernel/2046131
[2]: http://permalink.gmane.org/gmane.linux.kernel/2048070
[3]: http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/70031
[4]: http://permalink.gmane.org/gmane.linux.power-management.general/66143
[5]: http://permalink.gmane.org/gmane.linux.kernel.mm/138902
[6]: http://libdivecomputer.org/pipermail/devel/2014-July/000329.html

Regards
Andrzej

---
 .../tests/assign_signed_to_unsigned.cocci          | 45 ++++++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 scripts/coccinelle/tests/assign_signed_to_unsigned.cocci

diff --git a/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
new file mode 100644
index 0000000..efa4e83
--- /dev/null
+++ b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
@@ -0,0 +1,45 @@
+/// Assigning signed function result to unsigned variable can indicate error.
+/// To decrease number of false positives patch looks if after assignment
+/// there is also check for negative values of the result.
+///
+// Confidence: High
+// Copyright: (C) 2015 Andrzej Hajda, Samsung Electronics Co., Ltd. GPLv2.
+// URL: http://coccinelle.lip6.fr/
+// Options: --include-headers --all-includes
+
+virtual context
+virtual org
+virtual report
+
+@r@
+typedef bool, u8, u16, u32, u64, s8, s16, s32, s64;
+{char, short, int, long, long long, s8, s16, s32, s64} vs;
+{unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long,
+    size_t, bool, u8, u16, u32, u64} vu;
+position p;
+identifier f;
+statement S1, S2;
+expression e;
+@@
+
+*vu@p = f(...)@vs;
+... when != vu = e;
+if ( \( vu < 0 \| vu <= 0 \) ) S1 else S2
+
+@script:python depends on r && org@
+p << r.p;
+f << r.f;
+vu << r.vu;
+@@
+
+msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (vu, f)
+coccilib.org.print_todo(p[0], msg)
+
+@script:python depends on r && report@
+p << r.p;
+f << r.f;
+vu << r.vu;
+@@
+
+msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (vu, f)
+coccilib.report.print_report(p[0], msg)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234063 — Re: [PATCH v2] coccinelle: assign signed result to unsigned variable

On Mon, 28 Sep 2015, Andrzej Hajda wrote:

> Assigning signed function result to unsigned variable can indicate error.
> To decrease number of false positives patch looks if after assignment
> there is also check for negative values of the result.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> Hi Julia,
>
> Thanks for the hint. Now it looks much better.
> Summarizing this patch has found 20 problems and has 22 false positives [1][2].

Do you have some examples of the false positives?

julia

> unsigned_lesser_than_zero.cocci patch posted earlier has found
> 40 problems [3][4], and about 80 false positives if I remember correctly.
> Few patches were rejected, as developers likes code for testing variable range,
> even if its result is always true/false [5][6], but most of kernel patches are
> real bug fixes.
>
> Both patches tries to address similar issues, maybe it would be good to merge
> them? Especially as their results overlap.
> Additionally I thought about adding detecting range checks in
> unsigned_lesser_than_zero.cocci, to decrease number of false positives.
> Of course it could then miss real bugs. What do you think about it?
>
> [1]: http://permalink.gmane.org/gmane.linux.kernel/2046131
> [2]: http://permalink.gmane.org/gmane.linux.kernel/2048070
> [3]: http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/70031
> [4]: http://permalink.gmane.org/gmane.linux.power-management.general/66143
> [5]: http://permalink.gmane.org/gmane.linux.kernel.mm/138902
> [6]: http://libdivecomputer.org/pipermail/devel/2014-July/000329.html
>
> Regards
> Andrzej
>
> ---
>  .../tests/assign_signed_to_unsigned.cocci          | 45 ++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
>  create mode 100644 scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
>
> diff --git a/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
> new file mode 100644
> index 0000000..efa4e83
> --- /dev/null
> +++ b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
> @@ -0,0 +1,45 @@
> +/// Assigning signed function result to unsigned variable can indicate error.
> +/// To decrease number of false positives patch looks if after assignment
> +/// there is also check for negative values of the result.
> +///
> +// Confidence: High
> +// Copyright: (C) 2015 Andrzej Hajda, Samsung Electronics Co., Ltd. GPLv2.
> +// URL: http://coccinelle.lip6.fr/
> +// Options: --include-headers --all-includes
> +
> +virtual context
> +virtual org
> +virtual report
> +
> +@r@
> +typedef bool, u8, u16, u32, u64, s8, s16, s32, s64;
> +{char, short, int, long, long long, s8, s16, s32, s64} vs;
> +{unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long,
> +    size_t, bool, u8, u16, u32, u64} vu;
> +position p;
> +identifier f;
> +statement S1, S2;
> +expression e;
> +@@
> +
> +*vu@p = f(...)@vs;
> +... when != vu = e;
> +if ( \( vu < 0 \| vu <= 0 \) ) S1 else S2
> +
> +@script:python depends on r && org@
> +p << r.p;
> +f << r.f;
> +vu << r.vu;
> +@@
> +
> +msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (vu, f)
> +coccilib.org.print_todo(p[0], msg)
> +
> +@script:python depends on r && report@
> +p << r.p;
> +f << r.f;
> +vu << r.vu;
> +@@
> +
> +msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (vu, f)
> +coccilib.report.print_report(p[0], msg)
> --
> 1.9.1
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234072 — Re: [PATCH v2] coccinelle: assign signed result to unsigned variable

On 09/28/2015 01:32 PM, Julia Lawall wrote:
>
> On Mon, 28 Sep 2015, Andrzej Hajda wrote:
>
>> Assigning signed function result to unsigned variable can indicate error.
>> To decrease number of false positives patch looks if after assignment
>> there is also check for negative values of the result.
>>
>> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
>> ---
>> Hi Julia,
>>
>> Thanks for the hint. Now it looks much better.
>> Summarizing this patch has found 20 problems and has 22 false positives [1][2].
> Do you have some examples of the false positives?
./drivers/acpi/acpica/nsarguments.c:130:1: WARNING: Assigning signed result to
unsigned variable: required_param_count = METHOD_GET_ARG_COUNT(...)
./drivers/char/agp/intel-gtt.c:361:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = KB(...)
./drivers/char/agp/intel-gtt.c:364:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:367:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:382:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:385:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:388:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:391:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:394:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:397:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:400:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:403:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:406:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:409:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:412:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:415:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/char/agp/intel-gtt.c:418:3: WARNING: Assigning signed result to
unsigned variable: stolen_size = MB(...)
./drivers/input/touchscreen/cyttsp4_core.c:967:1: WARNING: Assigning signed
result to unsigned variable: num_cur_tch = GET_NUM_TOUCHES(...)
./drivers/pinctrl/freescale/pinctrl-imx.c:648:2: WARNING: Assigning signed
result to unsigned variable: nfuncs = of_get_child_count(...)
./fs/btrfs/file.c:1572:2: WARNING: Assigning signed result to unsigned variable:
copied = btrfs_copy_from_user(...)
./fs/xfs/libxfs/xfs_inode_fork.c:541:2: WARNING: Assigning signed result to
unsigned variable: new_size = XFS_BMAP_BROOT_SPACE_CALC(...)

As you see most of them are macros, of_get_child_count and btrfs_copy_from_user
return int but always non-negative.

Regards
Andrzej

>
> julia
>
>> unsigned_lesser_than_zero.cocci patch posted earlier has found
>> 40 problems [3][4], and about 80 false positives if I remember correctly.
>> Few patches were rejected, as developers likes code for testing variable range,
>> even if its result is always true/false [5][6], but most of kernel patches are
>> real bug fixes.
>>
>> Both patches tries to address similar issues, maybe it would be good to merge
>> them? Especially as their results overlap.
>> Additionally I thought about adding detecting range checks in
>> unsigned_lesser_than_zero.cocci, to decrease number of false positives.
>> Of course it could then miss real bugs. What do you think about it?
>>
>> [1]: http://permalink.gmane.org/gmane.linux.kernel/2046131
>> [2]: http://permalink.gmane.org/gmane.linux.kernel/2048070
>> [3]: http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/70031
>> [4]: http://permalink.gmane.org/gmane.linux.power-management.general/66143
>> [5]: http://permalink.gmane.org/gmane.linux.kernel.mm/138902
>> [6]: http://libdivecomputer.org/pipermail/devel/2014-July/000329.html
>>
>> Regards
>> Andrzej
>>
>> ---
>>  .../tests/assign_signed_to_unsigned.cocci          | 45 ++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>  create mode 100644 scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
>>
>> diff --git a/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
>> new file mode 100644
>> index 0000000..efa4e83
>> --- /dev/null
>> +++ b/scripts/coccinelle/tests/assign_signed_to_unsigned.cocci
>> @@ -0,0 +1,45 @@
>> +/// Assigning signed function result to unsigned variable can indicate error.
>> +/// To decrease number of false positives patch looks if after assignment
>> +/// there is also check for negative values of the result.
>> +///
>> +// Confidence: High
>> +// Copyright: (C) 2015 Andrzej Hajda, Samsung Electronics Co., Ltd. GPLv2.
>> +// URL: http://coccinelle.lip6.fr/
>> +// Options: --include-headers --all-includes
>> +
>> +virtual context
>> +virtual org
>> +virtual report
>> +
>> +@r@
>> +typedef bool, u8, u16, u32, u64, s8, s16, s32, s64;
>> +{char, short, int, long, long long, s8, s16, s32, s64} vs;
>> +{unsigned char, unsigned short, unsigned int, unsigned long, unsigned long long,
>> +    size_t, bool, u8, u16, u32, u64} vu;
>> +position p;
>> +identifier f;
>> +statement S1, S2;
>> +expression e;
>> +@@
>> +
>> +*vu@p = f(...)@vs;
>> +... when != vu = e;
>> +if ( \( vu < 0 \| vu <= 0 \) ) S1 else S2
>> +
>> +@script:python depends on r && org@
>> +p << r.p;
>> +f << r.f;
>> +vu << r.vu;
>> +@@
>> +
>> +msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (vu, f)
>> +coccilib.org.print_todo(p[0], msg)
>> +
>> +@script:python depends on r && report@
>> +p << r.p;
>> +f << r.f;
>> +vu << r.vu;
>> +@@
>> +
>> +msg = "WARNING: Assigning signed result to unsigned variable: %s = %s(...)" % (vu, f)
>> +coccilib.report.print_report(p[0], msg)
>> --
>> 1.9.1
>>
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web