diff mbox

linux-gen: sysinfo: check for overflow

Message ID 1477579368-7294-1-git-send-email-maxim.uvarov@linaro.org
State New
Headers show

Commit Message

Maxim Uvarov Oct. 27, 2016, 2:42 p.m. UTC 
In near future cpu mhz more likely will not overflow 64
bit value, but it makes sense to add  assert for
overflow check.
https://bugs.linaro.org/show_bug.cgi?id=2425

Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

---
 platform/linux-generic/arch/x86/odp_sysinfo_parse.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

-- 
2.7.1.250.gff4ea60

Comments

Bill Fischofer Oct. 27, 2016, 7:59 p.m. UTC | #1 
On Thu, Oct 27, 2016 at 9:42 AM, Maxim Uvarov <maxim.uvarov@linaro.org>
wrote:

> In near future cpu mhz more likely will not overflow 64

> bit value, but it makes sense to add  assert for

> overflow check.

> https://bugs.linaro.org/show_bug.cgi?id=2425

>

> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

> ---

>  platform/linux-generic/arch/x86/odp_sysinfo_parse.c | 7 ++++++-

>  1 file changed, 6 insertions(+), 1 deletion(-)

>

> diff --git a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

> b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

> index 96127ec..5aefd81 100644

> --- a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

> +++ b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

> @@ -5,7 +5,9 @@

>   */

>

>  #include <odp_internal.h>

> +#include <odp_debug_internal.h>

>  #include <string.h>

> +#include <limits.h>

>

>  int cpuinfo_parser(FILE *file, system_info_t *sysinfo)

>  {

> @@ -68,8 +70,11 @@ uint64_t odp_cpu_hz_current(int id)

>         }

>

>         fclose(file);

> -       if (mhz)

> +       if (mhz) {

> +               /* check for overflow */

> +               ODP_ASSERT((ULLONG_MAX / 1000000) > mhz);

>


I'm not sure that Coverity understands ODP_ASSERT. Have we tried running
this to see if it resolves the Coverity issue? If not, I'd rather just mark
it as a known false positive.


>                 return (uint64_t)(mhz * 1000000.0);

> +       }

>

>         return 0;

>  }

> --

> 2.7.1.250.gff4ea60

>

>
Maxim Uvarov Nov. 29, 2016, 5:25 p.m. UTC | #2 
Coverity also does not like such division:

*** CID 1382604:  Incorrect expression (UNINTENDED_INTEGER_DIVISION)
/platform/linux-generic/arch/x86/odp_sysinfo_parse.c: 75 in 
odp_cpu_hz_current()
69              }
70      }
71
72      fclose(file);
73      if (mhz) {
74              /* check for overflow */
 >>>     CID 1382604:  Incorrect expression (UNINTENDED_INTEGER_DIVISION)

 >>>     Dividing integer expressions "18446744073709551615ULL" and 

"1000000ULL", and then converting the integer quotient to type "double". 
Any remainder, or fractional part of the quotient, is ignored.
75              ODP_ASSERT((ULLONG_MAX / 1000000) > mhz);
76              return (uint64_t)(mhz * 1000000.0);
77      }
78
79      return 0;


Bill, looks like then it's better to go with original plan to make it 
false-positive.

Maxim.


On 10/27/16 17:42, Maxim Uvarov wrote:
> In near future cpu mhz more likely will not overflow 64

> bit value, but it makes sense to add  assert for

> overflow check.

> https://bugs.linaro.org/show_bug.cgi?id=2425

>

> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

> ---

>   platform/linux-generic/arch/x86/odp_sysinfo_parse.c | 7 ++++++-

>   1 file changed, 6 insertions(+), 1 deletion(-)

>

> diff --git a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

> index 96127ec..5aefd81 100644

> --- a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

> +++ b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

> @@ -5,7 +5,9 @@

>    */

>   

>   #include <odp_internal.h>

> +#include <odp_debug_internal.h>

>   #include <string.h>

> +#include <limits.h>

>   

>   int cpuinfo_parser(FILE *file, system_info_t *sysinfo)

>   {

> @@ -68,8 +70,11 @@ uint64_t odp_cpu_hz_current(int id)

>   	}

>   

>   	fclose(file);

> -	if (mhz)

> +	if (mhz) {

> +		/* check for overflow */

> +		ODP_ASSERT((ULLONG_MAX / 1000000) > mhz);

>   		return (uint64_t)(mhz * 1000000.0);

> +	}

>   

>   	return 0;

>   }
Bill Fischofer Nov. 29, 2016, 11:27 p.m. UTC | #3 
On Tue, Nov 29, 2016 at 11:25 AM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
> Coverity also does not like such division:

>

> *** CID 1382604:  Incorrect expression (UNINTENDED_INTEGER_DIVISION)

> /platform/linux-generic/arch/x86/odp_sysinfo_parse.c: 75 in

> odp_cpu_hz_current()

> 69              }

> 70      }

> 71

> 72      fclose(file);

> 73      if (mhz) {

> 74              /* check for overflow */

>>>>     CID 1382604:  Incorrect expression (UNINTENDED_INTEGER_DIVISION)

>>>>     Dividing integer expressions "18446744073709551615ULL" and

>>>> "1000000ULL", and then converting the integer quotient to type "double". Any

>>>> remainder, or fractional part of the quotient, is ignored.

> 75              ODP_ASSERT((ULLONG_MAX / 1000000) > mhz);

> 76              return (uint64_t)(mhz * 1000000.0);

> 77      }

> 78

> 79      return 0;

>

>

> Bill, looks like then it's better to go with original plan to make it

> false-positive.

>

> Maxim.


OK, done. Thanks.


>

>

>

> On 10/27/16 17:42, Maxim Uvarov wrote:

>>

>> In near future cpu mhz more likely will not overflow 64

>> bit value, but it makes sense to add  assert for

>> overflow check.

>> https://bugs.linaro.org/show_bug.cgi?id=2425

>>

>> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org>

>> ---

>>   platform/linux-generic/arch/x86/odp_sysinfo_parse.c | 7 ++++++-

>>   1 file changed, 6 insertions(+), 1 deletion(-)

>>

>> diff --git a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

>> b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

>> index 96127ec..5aefd81 100644

>> --- a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

>> +++ b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c

>> @@ -5,7 +5,9 @@

>>    */

>>     #include <odp_internal.h>

>> +#include <odp_debug_internal.h>

>>   #include <string.h>

>> +#include <limits.h>

>>     int cpuinfo_parser(FILE *file, system_info_t *sysinfo)

>>   {

>> @@ -68,8 +70,11 @@ uint64_t odp_cpu_hz_current(int id)

>>         }

>>         fclose(file);

>> -       if (mhz)

>> +       if (mhz) {

>> +               /* check for overflow */

>> +               ODP_ASSERT((ULLONG_MAX / 1000000) > mhz);

>>                 return (uint64_t)(mhz * 1000000.0);

>> +       }

>>         return 0;

>>   }

>

>
diff mbox

Patch

diff --git a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c
index 96127ec..5aefd81 100644
--- a/platform/linux-generic/arch/x86/odp_sysinfo_parse.c
+++ b/platform/linux-generic/arch/x86/odp_sysinfo_parse.c
@@ -5,7 +5,9 @@ 
  */
 
 #include <odp_internal.h>
+#include <odp_debug_internal.h>
 #include <string.h>
+#include <limits.h>
 
 int cpuinfo_parser(FILE *file, system_info_t *sysinfo)
 {
@@ -68,8 +70,11 @@  uint64_t odp_cpu_hz_current(int id)
 	}
 
 	fclose(file);
-	if (mhz)
+	if (mhz) {
+		/* check for overflow */
+		ODP_ASSERT((ULLONG_MAX / 1000000) > mhz);
 		return (uint64_t)(mhz * 1000000.0);
+	}
 
 	return 0;
 }