| Message ID | 1464286981-2052-1-git-send-email-maxim.uvarov@linaro.org |
|---|---|
| State | New |
| Headers | show |
please review, Maxim. On 05/26/16 21:23, Maxim Uvarov wrote: > https://bugs.linaro.org/show_bug.cgi?id=2253 > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/odp_system_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c > index fca35ce..0a09443 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void) > if (sscanf(str, "Hugepagesize: %8lu kB", &sz) == 1) { > ODP_DBG("defaut hp size is %" PRIu64 " kB\n", sz); > fclose(file); > - return (uint64_t)sz * 1024; > + return (uint64_t)sz * 1024ULL; > } > } >
On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > https://bugs.linaro.org/show_bug.cgi?id=2253 > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> > --- > platform/linux-generic/odp_system_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_system_info.c > b/platform/linux-generic/odp_system_info.c > index fca35ce..0a09443 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void) > if (sscanf(str, "Hugepagesize: %8lu kB", &sz) == 1) { > ODP_DBG("defaut hp size is %" PRIu64 " kB\n", sz); > fclose(file); > - return (uint64_t)sz * 1024; > + return (uint64_t)sz * 1024ULL; > I'm not sure that this is fixing the issue that Coverity is complaining about. What it's saying is that if sz is >= UINT64_MAX / 1024 then multiplying this expression by 1024 will cause an overflow. Coverity doesn't understand that sz will never be anywhere near that large. The solution would be to mask sz so that Coverity can see that the multiplication can never overflow. Or we could just mark this as a false positive. I think either would be acceptable. > } > } > > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org > https://lists.linaro.org/mailman/listinfo/lng-odp >
On 05/27/16 21:16, Bill Fischofer wrote: > > > On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > https://bugs.linaro.org/show_bug.cgi?id=2253 > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> > --- > platform/linux-generic/odp_system_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_system_info.c > b/platform/linux-generic/odp_system_info.c > index fca35ce..0a09443 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void) > if (sscanf(str, "Hugepagesize: %8lu kB", &sz) == > 1) { > ODP_DBG("defaut hp size is %" PRIu64 " > kB\n", sz); > fclose(file); > - return (uint64_t)sz * 1024; > + return (uint64_t)sz * 1024ULL; > > > I'm not sure that this is fixing the issue that Coverity is > complaining about. What it's saying is that if sz is >= UINT64_MAX / > 1024 then multiplying this expression by 1024 will cause an overflow. > Coverity doesn't understand that sz will never be anywhere near that > large. > > The solution would be to mask sz so that Coverity can see that the > multiplication can never overflow. Or we could just mark this as a > false positive. I think either would be acceptable. return (uint64_t)sz << 10 will work here? > > } > } > > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > https://lists.linaro.org/mailman/listinfo/lng-odp > >
On Fri, May 27, 2016 at 1:21 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/27/16 21:16, Bill Fischofer wrote: > >> >> >> On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> https://bugs.linaro.org/show_bug.cgi?id=2253 >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> >> --- >> platform/linux-generic/odp_system_info.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_system_info.c >> b/platform/linux-generic/odp_system_info.c >> index fca35ce..0a09443 100644 >> --- a/platform/linux-generic/odp_system_info.c >> +++ b/platform/linux-generic/odp_system_info.c >> @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void) >> if (sscanf(str, "Hugepagesize: %8lu kB", &sz) == >> 1) { >> ODP_DBG("defaut hp size is %" PRIu64 " >> kB\n", sz); >> fclose(file); >> - return (uint64_t)sz * 1024; >> + return (uint64_t)sz * 1024ULL; >> >> >> I'm not sure that this is fixing the issue that Coverity is complaining >> about. What it's saying is that if sz is >= UINT64_MAX / 1024 then >> multiplying this expression by 1024 will cause an overflow. Coverity >> doesn't understand that sz will never be anywhere near that large. >> >> The solution would be to mask sz so that Coverity can see that the >> multiplication can never overflow. Or we could just mark this as a false >> positive. I think either would be acceptable. >> > > return (uint64_t)sz << 10 > > will work here? > That would have the same result since the compiler already optimizes multiplications by known powers of two into shifts. What would likely keep Coverity happy would be something like: #define RANGE_MASK (UINT64_MAX / 1024) ... return ((uint64_t)sz & RANGE_MASK) * 1024; > >> } >> } >> >> -- >> 2.7.1.250.gff4ea60 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >
On 05/27/16 21:40, Bill Fischofer wrote: > > > On Fri, May 27, 2016 at 1:21 PM, Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>> wrote: > > On 05/27/16 21:16, Bill Fischofer wrote: > > > > On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov > <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> wrote: > > https://bugs.linaro.org/show_bug.cgi?id=2253 > > Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org> > <mailto:maxim.uvarov@linaro.org > <mailto:maxim.uvarov@linaro.org>>> > --- > platform/linux-generic/odp_system_info.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/platform/linux-generic/odp_system_info.c > b/platform/linux-generic/odp_system_info.c > index fca35ce..0a09443 100644 > --- a/platform/linux-generic/odp_system_info.c > +++ b/platform/linux-generic/odp_system_info.c > @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void) > if (sscanf(str, "Hugepagesize: %8lu kB", > &sz) == > 1) { > ODP_DBG("defaut hp size is %" PRIu64 " > kB\n", sz); > fclose(file); > - return (uint64_t)sz * 1024; > + return (uint64_t)sz * 1024ULL; > > > I'm not sure that this is fixing the issue that Coverity is > complaining about. What it's saying is that if sz is >= > UINT64_MAX / 1024 then multiplying this expression by 1024 > will cause an overflow. Coverity doesn't understand that sz > will never be anywhere near that large. > > The solution would be to mask sz so that Coverity can see that > the multiplication can never overflow. Or we could just mark > this as a false positive. I think either would be acceptable. > > > return (uint64_t)sz << 10 > > will work here? > > > That would have the same result since the compiler already optimizes > multiplications by known powers of two into shifts. > > What would likely keep Coverity happy would be something like: > > #define RANGE_MASK (UINT64_MAX / 1024) > > ... > > return ((uint64_t)sz & RANGE_MASK) * 1024; I think we should have such reading in number of places. Or we need to do some internal function to scan in megabytes or just set this as false positive in Coverity. Because it's parsed /proc there can not be number more than in unsigned long and uint64_t has to be enough for any case. Maxim. > > } > } > > -- > 2.7.1.250.gff4ea60 > > _______________________________________________ > lng-odp mailing list > lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> > <mailto:lng-odp@lists.linaro.org > <mailto:lng-odp@lists.linaro.org>> > https://lists.linaro.org/mailman/listinfo/lng-odp > > > >
On Fri, May 27, 2016 at 2:38 PM, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > On 05/27/16 21:40, Bill Fischofer wrote: > >> >> >> On Fri, May 27, 2016 at 1:21 PM, Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>> wrote: >> >> On 05/27/16 21:16, Bill Fischofer wrote: >> >> >> >> On Thu, May 26, 2016 at 1:23 PM, Maxim Uvarov >> <maxim.uvarov@linaro.org <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org>>> wrote: >> >> https://bugs.linaro.org/show_bug.cgi?id=2253 >> >> Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org >> <mailto:maxim.uvarov@linaro.org> >> <mailto:maxim.uvarov@linaro.org >> >> <mailto:maxim.uvarov@linaro.org>>> >> --- >> platform/linux-generic/odp_system_info.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/platform/linux-generic/odp_system_info.c >> b/platform/linux-generic/odp_system_info.c >> index fca35ce..0a09443 100644 >> --- a/platform/linux-generic/odp_system_info.c >> +++ b/platform/linux-generic/odp_system_info.c >> @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void) >> if (sscanf(str, "Hugepagesize: %8lu kB", >> &sz) == >> 1) { >> ODP_DBG("defaut hp size is %" PRIu64 " >> kB\n", sz); >> fclose(file); >> - return (uint64_t)sz * 1024; >> + return (uint64_t)sz * 1024ULL; >> >> >> I'm not sure that this is fixing the issue that Coverity is >> complaining about. What it's saying is that if sz is >= >> UINT64_MAX / 1024 then multiplying this expression by 1024 >> will cause an overflow. Coverity doesn't understand that sz >> will never be anywhere near that large. >> >> The solution would be to mask sz so that Coverity can see that >> the multiplication can never overflow. Or we could just mark >> this as a false positive. I think either would be acceptable. >> >> >> return (uint64_t)sz << 10 >> >> will work here? >> >> >> That would have the same result since the compiler already optimizes >> multiplications by known powers of two into shifts. >> >> What would likely keep Coverity happy would be something like: >> >> #define RANGE_MASK (UINT64_MAX / 1024) >> >> ... >> >> return ((uint64_t)sz & RANGE_MASK) * 1024; >> > > > I think we should have such reading in number of places. Or we need to do > some internal function to scan in megabytes or just set this as false > positive in Coverity. Because it's parsed /proc there can not be number > more than in unsigned long and uint64_t has to be enough for any case. > > Maxim. > I agree. I've marked this as a false positive in Coverity. We can close this bug as invalid. > > >> } >> } >> >> -- >> 2.7.1.250.gff4ea60 >> >> _______________________________________________ >> lng-odp mailing list >> lng-odp@lists.linaro.org <mailto:lng-odp@lists.linaro.org> >> <mailto:lng-odp@lists.linaro.org >> <mailto:lng-odp@lists.linaro.org>> >> https://lists.linaro.org/mailman/listinfo/lng-odp >> >> >> >> >> >
diff --git a/platform/linux-generic/odp_system_info.c b/platform/linux-generic/odp_system_info.c index fca35ce..0a09443 100644 --- a/platform/linux-generic/odp_system_info.c +++ b/platform/linux-generic/odp_system_info.c @@ -86,7 +86,7 @@ static uint64_t default_huge_page_size(void) if (sscanf(str, "Hugepagesize: %8lu kB", &sz) == 1) { ODP_DBG("defaut hp size is %" PRIu64 " kB\n", sz); fclose(file); - return (uint64_t)sz * 1024; + return (uint64_t)sz * 1024ULL; } }
https://bugs.linaro.org/show_bug.cgi?id=2253 Signed-off-by: Maxim Uvarov <maxim.uvarov@linaro.org> --- platform/linux-generic/odp_system_info.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)