[PATCH] coccinelle: assign signed result to unsigned variable

[PATCH] coccinelle: assign signed result to unsigned variable

[Andrzej Hajda]

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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

> +@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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[Andrzej Hajda]

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
>

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

>>> +@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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

> 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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

> 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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

> 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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

>> 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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

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
>

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

> 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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

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
>

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

>> * 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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

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

Re: [PATCH] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

>> 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

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

[Andrzej Hajda]

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

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

[Julia Lawall]

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
>
>

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

[Andrzej Hajda]

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
>>
>>

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

[Julia Lawall]

On Mon, 28 Sep 2015, Andrzej Hajda wrote:

> 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.

OK, perhaps we could just live with them...

julia

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

[SF Markus Elfring]

> +// Options: --include-headers --all-includes

How do you think about the reuse of the parameter "--recursive-includes" here?

Will the list of checked function calls become more complete then?

Regards,
Markus

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

[Andrzej Hajda]

On 09/28/2015 02:07 PM, SF Markus Elfring wrote:
>> +// Options: --include-headers --all-includes
> 
> How do you think about the reuse of the parameter "--recursive-includes" here?

Last time I have tried it, kernel source check took more than 10 hours, so I
gave up.

Regards
Andrzej

> 
> Will the list of checked function calls become more complete then?
> 
> 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
> 

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

[SF Markus Elfring]

>> How do you think about the reuse of the parameter "--recursive-includes" here?
> 
> Last time I have tried it, kernel source check took more than 10 hours,
> so I gave up.

This is interesting background information.

There are opportunities for more fine-tuning of such a source code analysis,
aren't there?

Regards,
Markus

Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

On Mon, 28 Sep 2015, SF Markus Elfring wrote:

> >> How do you think about the reuse of the parameter "--recursive-includes" here?
> >
> > Last time I have tried it, kernel source check took more than 10 hours,
> > so I gave up.
>
> This is interesting background information.
>
> There are opportunities for more fine-tuning of such a source code analysis,
> aren't there?

I guess parallelism would be helpful?  You could try the following
options:

-j n --chunksize 10 --recursive-includes --include-headers-for-types

where n is the number of cores that you want to use.  Parsed header files
will be cached within chunks.  I don't really know what is the best
chunksize.

julia


>
> Regards,
> Markus
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

> I guess parallelism would be helpful?
> You could try the following options:
> 
> -j n --chunksize 10 --recursive-includes --include-headers-for-types
> 
> where n is the number of cores that you want to use.

Has the make target "coccicheck" direct support for such special parameters?


> Parsed header files will be cached within chunks.

Can the Coccinelle software work together with a kind of "precompiled
header database"?

Regards,
Markus

Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

On Mon, 28 Sep 2015, SF Markus Elfring wrote:

> > I guess parallelism would be helpful?
> > You could try the following options:
> >
> > -j n --chunksize 10 --recursive-includes --include-headers-for-types
> >
> > where n is the number of cores that you want to use.
>
> Has the make target "coccicheck" direct support for such special parameters?
>
>
> > Parsed header files will be cached within chunks.
>
> Can the Coccinelle software work together with a kind of "precompiled
> header database"?

There is an option --use-cache, which caches the compiled code on the
disk.  But I have not found the effects to be very satisfactory.  It is
still necessary to read in the serialized code.

julia

Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned variable

[SF Markus Elfring]

>> Can the Coccinelle software work together with a kind of "precompiled
>> header database"?
> 
> There is an option --use-cache, which caches the compiled code
> on the disk.  But I have not found the effects to be very satisfactory.

I am curious if this situation will be improved by further software evolution.

Regards,
Markus

Re: [Cocci] [PATCH v2] coccinelle: assign signed result to unsigned variable

[Julia Lawall]

On Mon, 28 Sep 2015, SF Markus Elfring wrote:

> >> Can the Coccinelle software work together with a kind of "precompiled
> >> header database"?
> >
> > There is an option --use-cache, which caches the compiled code
> > on the disk.  But I have not found the effects to be very satisfactory.
>
> I am curious if this situation will be improved by further software evolution.

There are no plans in that direction.

julia

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

[Julia Lawall]

Some comments:

If you get 20 good results and 22 false positives, I'm not sure whether 
high confidence is justified.  That seemes more like moderate confidence.

On the other hand, I think it is possible to get rid of the false 
positives.  The false positives are coming from the fact that you have:

if ( \( vu < 0 \| vu <= 0 \) ) S1 else S2

This can be flipped around to

if ( ! \( vu < 0 \| vu <= 0 \) ) S2 else S1

and then when we propagate the ! into the disjunction, we get v >= 0 for 
the first condition and v > 0 for the second condition.  v >= 0 is always 
true, so it could be reasonable to highlight it, but v > 0 is a perfectly 
reasonable test for an unsigned value, and is where you are getting the 
false positives from.  If you want to get rid of both v >= 0 and v < 0 
then you can just put disable neg_if in the initial @@, just after r, ie

@r disable neg_if@

On the other hand, if you want to keep the warning on v >= 0 but drop the 
warning on v > 0, then you will have to split the rules and put the 
disable neg_if on the one for v <= 0.

I think it would also be reasonable to merge the proposed semantic 
patches.  I guess this one gives most of the results anyway?

With recursive_includes, I got 70 results, at least 20 of which should be 
false positives due to the MB case.

julia