| Message ID | 1452646350-11999-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed 13 Jan 2016 01:52:29 AM CET, Fam Zheng wrote: > The implicit casting from unsigned int to double changes negative values > into large positive numbers and accepts them. We should instead print > an error. > > Check the number range so this case is catched and reported. I still don't know why qemu_opt_get_number() convert silently negative numbers into positive ones, shouldn't it just fail with an "invalid parameter" error? > +#define THROTTLE_VALUE_MAX 1000000000000000L This is larger than LONG_MAX in 32-bit systems, I don't know if you need to use LL instead. Berto
On Wed, 01/13 11:17, Alberto Garcia wrote: > On Wed 13 Jan 2016 01:52:29 AM CET, Fam Zheng wrote: > > > The implicit casting from unsigned int to double changes negative values > > into large positive numbers and accepts them. We should instead print > > an error. > > > > Check the number range so this case is catched and reported. > > I still don't know why qemu_opt_get_number() convert silently negative > numbers into positive ones, shouldn't it just fail with an "invalid > parameter" error? Because the parsing is done with strtoull(3) and unfortunately its man page says "negative values are considered valid input and are silently converted to the equivalent unsigned long int value." > > > +#define THROTTLE_VALUE_MAX 1000000000000000L > > This is larger than LONG_MAX in 32-bit systems, I don't know if you need > to use LL instead. I assume a compiler will handle that okay but yes it's safer to use LL. Fam
On Wed 13 Jan 2016 12:02:00 PM CET, Fam Zheng wrote: >> > Check the number range so this case is catched and reported. >> >> I still don't know why qemu_opt_get_number() convert silently >> negative numbers into positive ones, shouldn't it just fail with an >> "invalid parameter" error? > > Because the parsing is done with strtoull(3) and unfortunately its man > page says "negative values are considered valid input and are silently > converted to the equivalent unsigned long int value." I see... parse_uint() from cutils.c handles that by making an explicit check for negative numbers. It probably makes sense to apply the same solution (or even merge the code to the extent to which it's possible). I also noticed that there's a couple of places where we're calling qemu_opt_get_number() passing -1 as default value, so maybe that API needs to be reviewed anyway. Berto
On Wed, 01/13 12:13, Alberto Garcia wrote: > On Wed 13 Jan 2016 12:02:00 PM CET, Fam Zheng wrote: > > >> > Check the number range so this case is catched and reported. > >> > >> I still don't know why qemu_opt_get_number() convert silently > >> negative numbers into positive ones, shouldn't it just fail with an > >> "invalid parameter" error? > > > > Because the parsing is done with strtoull(3) and unfortunately its man > > page says "negative values are considered valid input and are silently > > converted to the equivalent unsigned long int value." > > I see... parse_uint() from cutils.c handles that by making an explicit > check for negative numbers. It probably makes sense to apply the same > solution (or even merge the code to the extent to which it's possible). > > I also noticed that there's a couple of places where we're calling > qemu_opt_get_number() passing -1 as default value, so maybe that API > needs to be reviewed anyway. Those callers rely on casting preserves the MSB as the sign, but that's ugly. Anyway I'd leave the API change for a separate series and keep this patch local to fix this particular regression. :) Fam
On 01/13/2016 03:17 AM, Alberto Garcia wrote: > On Wed 13 Jan 2016 01:52:29 AM CET, Fam Zheng wrote: > >> The implicit casting from unsigned int to double changes negative values >> into large positive numbers and accepts them. We should instead print >> an error. >> >> Check the number range so this case is catched and reported. s/catched/caught/ > > I still don't know why qemu_opt_get_number() convert silently negative > numbers into positive ones, shouldn't it just fail with an "invalid > parameter" error? Passing -1 as a synonym for ULLONG_MAX can be convenient. But rejecting it outright rather than doing wraparound wouldn't hurt libvirt too badly. > >> +#define THROTTLE_VALUE_MAX 1000000000000000L > > This is larger than LONG_MAX in 32-bit systems, I don't know if you need > to use LL instead. You do need LL, not for C99, but for older compilers (hello mingw).
diff --git a/blockdev.c b/blockdev.c index 2df0c6d..1afef87 100644 --- a/blockdev.c +++ b/blockdev.c @@ -348,7 +348,8 @@ static bool check_throttle_config(ThrottleConfig *cfg, Error **errp) } if (!throttle_is_valid(cfg)) { - error_setg(errp, "bps/iops/maxs values must be 0 or greater"); + error_setg(errp, "bps/iops/maxs values must be within [0, %" PRId64 + ")", (int64_t)THROTTLE_VALUE_MAX); return false; } diff --git a/include/qemu/throttle.h b/include/qemu/throttle.h index 12faaad..b5ddddd 100644 --- a/include/qemu/throttle.h +++ b/include/qemu/throttle.h @@ -29,6 +29,8 @@ #include "qemu-common.h" #include "qemu/timer.h" +#define THROTTLE_VALUE_MAX 1000000000000000L + typedef enum { THROTTLE_BPS_TOTAL, THROTTLE_BPS_READ, diff --git a/util/throttle.c b/util/throttle.c index 1113671..af4bc95 100644 --- a/util/throttle.c +++ b/util/throttle.c @@ -282,22 +282,18 @@ bool throttle_conflicting(ThrottleConfig *cfg) */ bool throttle_is_valid(ThrottleConfig *cfg) { - bool invalid = false; int i; for (i = 0; i < BUCKETS_COUNT; i++) { - if (cfg->buckets[i].avg < 0) { - invalid = true; + if (cfg->buckets[i].avg < 0 || + cfg->buckets[i].max < 0 || + cfg->buckets[i].avg > THROTTLE_VALUE_MAX || + cfg->buckets[i].max > THROTTLE_VALUE_MAX) { + return false; } } - for (i = 0; i < BUCKETS_COUNT; i++) { - if (cfg->buckets[i].max < 0) { - invalid = true; - } - } - - return !invalid; + return true; } /* check if bps_max/iops_max is used without bps/iops
The implicit casting from unsigned int to double changes negative values into large positive numbers and accepts them. We should instead print an error. Check the number range so this case is catched and reported. Signed-off-by: Fam Zheng <famz@redhat.com> --- blockdev.c | 3 ++- include/qemu/throttle.h | 2 ++ util/throttle.c | 16 ++++++---------- 3 files changed, 10 insertions(+), 11 deletions(-)