Re: [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str()

[Eduardo Habkost]

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