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

[Peter Maydell]

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