diff mbox series

util/cutils: Silent Coverity array overrun warning in freq_to_str()

Message ID 20201029185506.1241912-1-f4bug@amsat.org (mailing list archive)
State New, archived
Headers show
Series util/cutils: Silent Coverity array overrun warning in freq_to_str() | expand

Commit Message

Philippe Mathieu-Daudé Oct. 29, 2020, 6:55 p.m. UTC 
The biggest input value freq_to_str() can accept is UINT64_MAX,
which is ~18.446 EHz, less than 1000 EHz.
Add an assertion to help Coverity.

This silents CID 1435957:  Memory - illegal accesses (OVERRUN):

>>> Overrunning array "suffixes" of 7 8-byte elements at element
    index 7 (byte offset 63) using index "idx" (which evaluates to 7).

Reported-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 util/cutils.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Philippe Mathieu-Daudé Oct. 29, 2020, 6:56 p.m. UTC | #1 
Patch aiming the 5.2 release.

On Thu, Oct 29, 2020 at 7:55 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The biggest input value freq_to_str() can accept is UINT64_MAX,
> which is ~18.446 EHz, less than 1000 EHz.
> Add an assertion to help Coverity.
>
> This silents CID 1435957:  Memory - illegal accesses (OVERRUN):
>
> >>> Overrunning array "suffixes" of 7 8-byte elements at element
>     index 7 (byte offset 63) using index "idx" (which evaluates to 7).
>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  util/cutils.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/util/cutils.c b/util/cutils.c
> index c395974fab4..69c0ad7f888 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -891,6 +891,7 @@ char *freq_to_str(uint64_t freq_hz)
>      double freq = freq_hz;
>      size_t idx = 0;
>
> +    assert(freq <= UINT64_MAX); /* Max 64-bit value is less than 1000 EHz */
>      while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
>          freq /= 1000.0;
>          idx++;
> --
> 2.26.2
>
Peter Maydell Oct. 29, 2020, 7:13 p.m. UTC | #2 
On Thu, 29 Oct 2020 at 18:57, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> The biggest input value freq_to_str() can accept is UINT64_MAX,
> which is ~18.446 EHz, less than 1000 EHz.
> Add an assertion to help Coverity.
>
> This silents CID 1435957:  Memory - illegal accesses (OVERRUN):
>
> >>> Overrunning array "suffixes" of 7 8-byte elements at element
>     index 7 (byte offset 63) using index "idx" (which evaluates to 7).
>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  util/cutils.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/util/cutils.c b/util/cutils.c
> index c395974fab4..69c0ad7f888 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -891,6 +891,7 @@ char *freq_to_str(uint64_t freq_hz)
>      double freq = freq_hz;
>      size_t idx = 0;
>
> +    assert(freq <= UINT64_MAX); /* Max 64-bit value is less than 1000 EHz */
>      while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
>          freq /= 1000.0;
>          idx++;

I don't think I really agree with this as the way to silence
Coverity. The reason Coverity complains is because in
one part of the function you have code that says "it's possible
for idx to be >= ARRAY_SIZE(suffixes)" (because you test
that in the while loop condition) but then in the following
part (where you dereference you have code that says "it's not
possible for idx to be >= ARRAY_SIZE(suffixes)" (because
you dereference suffixes[idx]).

We should either consistently assume that idx can never
get to 7 (ie don't check it in the while loop condition
because that test can never return true) or we should
consistently guard against it happening (by switching the
while loop to "<=", or by handling the idx==ARRAY_SIZE(suffixes)
case specially.)

I think I would go for:
 * remove the test from the while condition
 * after the while loop, assert(idx < ARRAY_SIZE(suffixes));

thanks
-- PMM
Peter Maydell Oct. 29, 2020, 7:16 p.m. UTC | #3 
On Thu, 29 Oct 2020 at 19:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> We should either consistently assume that idx can never
> get to 7 (ie don't check it in the while loop condition
> because that test can never return true) or we should
> consistently guard against it happening (by switching the
> while loop to "<=", or by handling the idx==ARRAY_SIZE(suffixes)
> case specially.)

Oops; "switching to <=" isn't the right thing; you'd need to switch
to "< ARRAY_SIZE(suffixes) - 1". Anyway I think we should
do the other of the two options, not this one.

-- PMM
Eduardo Habkost Oct. 29, 2020, 8:26 p.m. UTC | #4 
On Thu, Oct 29, 2020 at 07:55:06PM +0100, Philippe Mathieu-Daudé wrote:
> The biggest input value freq_to_str() can accept is UINT64_MAX,
> which is ~18.446 EHz, less than 1000 EHz.
> Add an assertion to help Coverity.
> 
> This silents CID 1435957:  Memory - illegal accesses (OVERRUN):
> 
> >>> Overrunning array "suffixes" of 7 8-byte elements at element
>     index 7 (byte offset 63) using index "idx" (which evaluates to 7).
> 
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  util/cutils.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/util/cutils.c b/util/cutils.c
> index c395974fab4..69c0ad7f888 100644
> --- a/util/cutils.c
> +++ b/util/cutils.c
> @@ -891,6 +891,7 @@ char *freq_to_str(uint64_t freq_hz)
>      double freq = freq_hz;
>      size_t idx = 0;
>  
> +    assert(freq <= UINT64_MAX); /* Max 64-bit value is less than 1000 EHz */

If Coverity is really able to conclude that this assert really
ensures idx will never be out of bounds, I will be very
impressed.

>      while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {

I don't understand why the (idx < ARRAY_SIZE(suffix)) expression
above exists, if the code in this function will only work
correctly if the expression never becomes false.

It sounds simpler and more obvious to fix the boundary check.
In other words:

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
---
diff --git a/util/cutils.c b/util/cutils.c
index c395974fab..0d9261e1e5 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -891,7 +891,7 @@ char *freq_to_str(uint64_t freq_hz)
     double freq = freq_hz;
     size_t idx = 0;
 
-    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
+    while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes) - 1) {
         freq /= 1000.0;
         idx++;
     }
Eduardo Habkost Oct. 29, 2020, 8:40 p.m. UTC | #5 
On Thu, Oct 29, 2020 at 07:16:35PM +0000, Peter Maydell wrote:
> On Thu, 29 Oct 2020 at 19:13, Peter Maydell <peter.maydell@linaro.org> wrote:
> > We should either consistently assume that idx can never
> > get to 7 (ie don't check it in the while loop condition
> > because that test can never return true) or we should
> > consistently guard against it happening (by switching the
> > while loop to "<=", or by handling the idx==ARRAY_SIZE(suffixes)
> > case specially.)
> 
> Oops; "switching to <=" isn't the right thing; you'd need to switch
> to "< ARRAY_SIZE(suffixes) - 1". Anyway I think we should
> do the other of the two options, not this one.

"< ARRAY_SIZE(suffixes) - 1" patch submitted at
https://lore.kernel.org/qemu-devel/20201029203850.434351-1-ehabkost@redhat.com/
diff mbox series

Patch

diff --git a/util/cutils.c b/util/cutils.c
index c395974fab4..69c0ad7f888 100644
--- a/util/cutils.c
+++ b/util/cutils.c
@@ -891,6 +891,7 @@  char *freq_to_str(uint64_t freq_hz)
     double freq = freq_hz;
     size_t idx = 0;
 
+    assert(freq <= UINT64_MAX); /* Max 64-bit value is less than 1000 EHz */
     while (freq >= 1000.0 && idx < ARRAY_SIZE(suffixes)) {
         freq /= 1000.0;
         idx++;