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 |
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 >
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
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
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++; }
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 --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++;