If bkt->max == 0 and bkt->burst_length > 1 then we could have a
division by 0 in throttle_do_compute_wait(). That configuration is
however not permitted and is already detected by throttle_is_valid(),
but let's assert it in throttle_compute_wait() to make it explicit.
Found by Coverity (CID: 1381016).
Signed-off-by: Alberto Garcia <berto@igalia.com>
---
util/throttle.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/util/throttle.c b/util/throttle.c
index 06bf916adc..b38e742da5 100644
--- a/util/throttle.c
+++ b/util/throttle.c
@@ -124,6 +124,7 @@ int64_t throttle_compute_wait(LeakyBucket *bkt)
/* If the main bucket is not full yet we still have to check the
* burst bucket in order to enforce the burst limit */
if (bkt->burst_length > 1) {
+ assert(bkt->max > 0); /* see throttle_is_valid() */
extra = bkt->burst_level - burst_bucket_size;
if (extra > 0) {
return throttle_do_compute_wait(bkt->max, extra);
--
2.11.0
On 09/13/2017 03:28 AM, Alberto Garcia wrote: > If bkt->max == 0 and bkt->burst_length > 1 then we could have a > division by 0 in throttle_do_compute_wait(). That configuration is > however not permitted and is already detected by throttle_is_valid(), > but let's assert it in throttle_compute_wait() to make it explicit. > > Found by Coverity (CID: 1381016). > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > util/throttle.c | 1 + > 1 file changed, 1 insertion(+) Reviewed-by: Eric Blake <eblake@redhat.com> > > diff --git a/util/throttle.c b/util/throttle.c > index 06bf916adc..b38e742da5 100644 > --- a/util/throttle.c > +++ b/util/throttle.c > @@ -124,6 +124,7 @@ int64_t throttle_compute_wait(LeakyBucket *bkt) > /* If the main bucket is not full yet we still have to check the > * burst bucket in order to enforce the burst limit */ > if (bkt->burst_length > 1) { > + assert(bkt->max > 0); /* see throttle_is_valid() */ > extra = bkt->burst_level - burst_bucket_size; > if (extra > 0) { > return throttle_do_compute_wait(bkt->max, extra); > -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Hi Alberto, On 09/13/2017 05:28 AM, Alberto Garcia wrote: > If bkt->max == 0 and bkt->burst_length > 1 then we could have a > division by 0 in throttle_do_compute_wait(). That configuration is > however not permitted and is already detected by throttle_is_valid(), > but let's assert it in throttle_compute_wait() to make it explicit. This is correct but I'm not sure this is enough, as throttle_compute_wait() is exported/public, however it seems testing is the only reason to export it. Also I spent 10min looking at it thinking about how bkt->max is used, before to realize there should be a simpler way to write this (KISS). Anyway I'm queuing your patch in my /static-analysis branch I plan to this week where I use a self-explanatory macro instead of assert() and will see if I can simplify it (note that I'm not a maintainer!). Regards, Phil. > > Found by Coverity (CID: 1381016). > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > util/throttle.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/util/throttle.c b/util/throttle.c > index 06bf916adc..b38e742da5 100644 > --- a/util/throttle.c > +++ b/util/throttle.c > @@ -124,6 +124,7 @@ int64_t throttle_compute_wait(LeakyBucket *bkt) > /* If the main bucket is not full yet we still have to check the > * burst bucket in order to enforce the burst limit */ > if (bkt->burst_length > 1) { > + assert(bkt->max > 0); /* see throttle_is_valid() */ > extra = bkt->burst_level - burst_bucket_size; > if (extra > 0) { > return throttle_do_compute_wait(bkt->max, extra); >
On Wed 13 Sep 2017 06:31:58 PM CEST, Philippe Mathieu-Daudé wrote: >> If bkt->max == 0 and bkt->burst_length > 1 then we could have a >> division by 0 in throttle_do_compute_wait(). That configuration is >> however not permitted and is already detected by throttle_is_valid(), >> but let's assert it in throttle_compute_wait() to make it explicit. > > This is correct but I'm not sure this is enough, as > throttle_compute_wait() is exported/public, however it seems testing > is the only reason to export it. You're right but in general I don't think the throttling code is guaranteed to behave correctly if the configuration hasn't been checked with throttle_is_valid() first. > Also I spent 10min looking at it thinking about how bkt->max is used, > before to realize there should be a simpler way to write this (KISS). I'm sure there is :) if you have suggestions I'll be glad to hear them. Berto
Am 13.09.2017 um 10:28 hat Alberto Garcia geschrieben: > If bkt->max == 0 and bkt->burst_length > 1 then we could have a > division by 0 in throttle_do_compute_wait(). That configuration is > however not permitted and is already detected by throttle_is_valid(), > but let's assert it in throttle_compute_wait() to make it explicit. > > Found by Coverity (CID: 1381016). > > Signed-off-by: Alberto Garcia <berto@igalia.com> Thanks, applied to the block branch. Kevin
© 2016 - 2024 Red Hat, Inc.