* [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str()
@ 2020-10-29 18:55 Philippe Mathieu-Daudé
2020-10-29 18:56 ` [PATCH-for-5.2] " Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-29 18:55 UTC (permalink / raw)
To: qemu-devel
Cc: Luc Michel, Eduardo Habkost, Philippe Mathieu-Daudé,
Alistair Francis, Richard Henderson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH-for-5.2] util/cutils: Silent Coverity array overrun warning in freq_to_str()
2020-10-29 18:55 [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str() Philippe Mathieu-Daudé
@ 2020-10-29 18:56 ` Philippe Mathieu-Daudé
2020-10-29 19:13 ` [PATCH] " Peter Maydell
2020-10-29 20:26 ` Eduardo Habkost
2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-10-29 18:56 UTC (permalink / raw)
To: qemu-devel@nongnu.org Developers
Cc: Luc Michel, Alistair Francis, Eduardo Habkost, Richard Henderson
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str()
2020-10-29 18:55 [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str() Philippe Mathieu-Daudé
2020-10-29 18:56 ` [PATCH-for-5.2] " Philippe Mathieu-Daudé
@ 2020-10-29 19:13 ` Peter Maydell
2020-10-29 19:16 ` Peter Maydell
2020-10-29 20:26 ` Eduardo Habkost
2 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2020-10-29 19:13 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Richard Henderson, Luc Michel, QEMU Developers,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str()
2020-10-29 19:13 ` [PATCH] " Peter Maydell
@ 2020-10-29 19:16 ` Peter Maydell
2020-10-29 20:40 ` Eduardo Habkost
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2020-10-29 19:16 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Richard Henderson, Luc Michel, QEMU Developers,
Eduardo Habkost
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str()
2020-10-29 18:55 [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str() Philippe Mathieu-Daudé
2020-10-29 18:56 ` [PATCH-for-5.2] " Philippe Mathieu-Daudé
2020-10-29 19:13 ` [PATCH] " Peter Maydell
@ 2020-10-29 20:26 ` Eduardo Habkost
2 siblings, 0 replies; 6+ messages in thread
From: Eduardo Habkost @ 2020-10-29 20:26 UTC (permalink / raw)
To: Philippe Mathieu-Daudé
Cc: Alistair Francis, Luc Michel, qemu-devel, Richard Henderson
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str()
2020-10-29 19:16 ` Peter Maydell
@ 2020-10-29 20:40 ` Eduardo Habkost
0 siblings, 0 replies; 6+ messages in thread
From: Eduardo Habkost @ 2020-10-29 20:40 UTC (permalink / raw)
To: Peter Maydell
Cc: Alistair Francis, Richard Henderson, Luc Michel,
Philippe Mathieu-Daudé, QEMU Developers
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/
--
Eduardo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-29 20:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-10-29 18:55 [PATCH] util/cutils: Silent Coverity array overrun warning in freq_to_str() Philippe Mathieu-Daudé
2020-10-29 18:56 ` [PATCH-for-5.2] " Philippe Mathieu-Daudé
2020-10-29 19:13 ` [PATCH] " Peter Maydell
2020-10-29 19:16 ` Peter Maydell
2020-10-29 20:40 ` Eduardo Habkost
2020-10-29 20:26 ` Eduardo Habkost
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).