[Qemu-devel] [PATCH] throttle: Assert that bkt->max is valid in throttle_compute_wait()

[Qemu-devel] [PATCH] throttle: Assert that bkt->max is valid in throttle_compute_wait()

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH] throttle: Assert that bkt->max is valid in throttle_compute_wait()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1282 bytes --]

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 619 bytes --]

Re: [Qemu-devel] [PATCH] throttle: Assert that bkt->max is valid in throttle_compute_wait()

[Philippe Mathieu-Daudé]

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);
> 

Re: [Qemu-devel] [PATCH] throttle: Assert that bkt->max is valid in throttle_compute_wait()

[Alberto Garcia]

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

Re: [Qemu-devel] [Qemu-block] [PATCH] throttle: Assert that bkt->max is valid in throttle_compute_wait()

[Kevin Wolf]

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