throttle: Assert that bkt->max is valid in throttle_compute_wait()
PATCH 1/1 throttle: Assert that bkt->max is valid in throttle_compute_wait()
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).
--- util/throttle.c | 1 + 1 file changed, 1 insertion(+)
util/throttle.c +1 -0
@@ -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);
Signature · 1 line
2.11.0
Quoted 11 lines · Alberto Garcia
If
bkt->max== 0 andbkt->burst_length> 1 then we could have a division by 0 inthrottle_do_compute_wait(). That configuration is however not permitted and is already detected bythrottle_is_valid(), but let's assert it inthrottle_compute_wait()to make it explicit.Found by Coverity (CID: 1381016).
Signed-off-by Alberto Garcia <[email protected]>--- util/throttle.c | 1 + 1 file changed, 1 insertion(+)
Quoted 14 lines
util/throttle.c +1
-0@@ -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);
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Hi Alberto,
Quoted 4 lines · Alberto Garcia
If
bkt->max== 0 andbkt->burst_length> 1 then we could have a division by 0 inthrottle_do_compute_wait(). That configuration is however not permitted and is already detected bythrottle_is_valid(), but let's assert it inthrottle_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.
Quoted 21 lines
Found by Coverity (CID: 1381016).
Signed-off-by Alberto Garcia <[email protected]>--- util/throttle.c | 1 + 1 file changed, 1 insertion(+)util/throttle.c +1
-0@@ -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);
Quoted 8 lines · Philippe Mathieu-Daudé
Quoted 4 lines
If
bkt->max== 0 andbkt->burst_length> 1 then we could have a division by 0 inthrottle_do_compute_wait(). That configuration is however not permitted and is already detected bythrottle_is_valid(), but let's assert it inthrottle_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.
Quoted 2 lines
Also I spent 10min looking at it thinking about how
bkt->maxis 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
Kevin Wolf
AppliedAm 13.09.2017 um 10:28 hat Alberto Garcia geschrieben:
Quoted 8 lines
If
bkt->max== 0 andbkt->burst_length> 1 then we could have a division by 0 inthrottle_do_compute_wait(). That configuration is however not permitted and is already detected bythrottle_is_valid(), but let's assert it inthrottle_compute_wait()to make it explicit.Found by Coverity (CID: 1381016).
Signed-off-by Alberto Garcia <[email protected]>
Thanks, applied to the block branch.
Kevin