| Message ID | 1375338695-670-1-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/01/2013 12:31 AM, Fam Zheng wrote: > Fix it by calling strtoll instead, which will report ERANGE as expected. > > (HMP) block_set_io_throttle ide0-hd0 999999999999999999 0 0 0 0 0 > (HMP) block_set_io_throttle ide0-hd0 9999999999999999999 0 0 0 0 0 > number too large > (HMP) block_set_io_throttle ide0-hd0 99999999999999999999 0 0 0 0 0 > number too large Your change causes this error message: (HMP) block_set_io_throttle ide0-hd0 -99999999999999999999 0 0 0 0 0 number too large Does the "too large" mean in magnitude (correct message) or in value (misleading message, as any negative number is smaller in value than our minimum of 0)? > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > monitor.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/monitor.c b/monitor.c > index 5dc0aa9..7bfb469 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -3286,7 +3286,7 @@ static int64_t expr_unary(Monitor *mon) > break; > default: > errno = 0; > - n = strtoull(pch, &p, 0); > + n = strtoll(pch, &p, 0); I'm worried that this will break callers that treat their argument as unsigned, and where the full range of unsigned input was desirable. At this point, it's probably safer to do a case-by-case analysis of all callers that use expr_unary() to decide which callers must reject negative values, instead of making the parser reject numbers that it previously accepted, thus changing the behavior of callers that treated the result as unsigned.
On Thu, 01 Aug 2013 07:52:17 -0600 Eric Blake <eblake@redhat.com> wrote: > On 08/01/2013 12:31 AM, Fam Zheng wrote: > > Fix it by calling strtoll instead, which will report ERANGE as expected. > > > > (HMP) block_set_io_throttle ide0-hd0 999999999999999999 0 0 0 0 0 > > (HMP) block_set_io_throttle ide0-hd0 9999999999999999999 0 0 0 0 0 > > number too large > > (HMP) block_set_io_throttle ide0-hd0 99999999999999999999 0 0 0 0 0 > > number too large > > Your change causes this error message: > (HMP) block_set_io_throttle ide0-hd0 -99999999999999999999 0 0 0 0 0 > number too large > > Does the "too large" mean in magnitude (correct message) or in value > (misleading message, as any negative number is smaller in value than our > minimum of 0)? > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > monitor.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/monitor.c b/monitor.c > > index 5dc0aa9..7bfb469 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -3286,7 +3286,7 @@ static int64_t expr_unary(Monitor *mon) > > break; > > default: > > errno = 0; > > - n = strtoull(pch, &p, 0); > > + n = strtoll(pch, &p, 0); > > I'm worried that this will break callers that treat their argument as > unsigned, and where the full range of unsigned input was desirable. At > this point, it's probably safer to do a case-by-case analysis of all > callers that use expr_unary() to decide which callers must reject > negative values, instead of making the parser reject numbers that it > previously accepted, thus changing the behavior of callers that treated > the result as unsigned. > Fam, what motivated this change? Is anyone entering such big numbers for block_set_io_throttle?
On Thu, 08/01 10:00, Luiz Capitulino wrote: > On Thu, 01 Aug 2013 07:52:17 -0600 > Eric Blake <eblake@redhat.com> wrote: > > > On 08/01/2013 12:31 AM, Fam Zheng wrote: > > > Fix it by calling strtoll instead, which will report ERANGE as expected. > > > > > > (HMP) block_set_io_throttle ide0-hd0 999999999999999999 0 0 0 0 0 > > > (HMP) block_set_io_throttle ide0-hd0 9999999999999999999 0 0 0 0 0 > > > number too large > > > (HMP) block_set_io_throttle ide0-hd0 99999999999999999999 0 0 0 0 0 > > > number too large > > > > Your change causes this error message: > > (HMP) block_set_io_throttle ide0-hd0 -99999999999999999999 0 0 0 0 0 > > number too large > > > > Does the "too large" mean in magnitude (correct message) or in value > > (misleading message, as any negative number is smaller in value than our > > minimum of 0)? > > > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > --- > > > monitor.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/monitor.c b/monitor.c > > > index 5dc0aa9..7bfb469 100644 > > > --- a/monitor.c > > > +++ b/monitor.c > > > @@ -3286,7 +3286,7 @@ static int64_t expr_unary(Monitor *mon) > > > break; > > > default: > > > errno = 0; > > > - n = strtoull(pch, &p, 0); > > > + n = strtoll(pch, &p, 0); > > > > I'm worried that this will break callers that treat their argument as > > unsigned, and where the full range of unsigned input was desirable. At > > this point, it's probably safer to do a case-by-case analysis of all > > callers that use expr_unary() to decide which callers must reject > > negative values, instead of making the parser reject numbers that it > > previously accepted, thus changing the behavior of callers that treated > > the result as unsigned. > > > > Fam, what motivated this change? Is anyone entering such big numbers > for block_set_io_throttle? It's for: https://bugzilla.redhat.com/show_bug.cgi?id=988701#c1 In practice I don't think anyone entering such big numbers, it's corner case. But as this seems an expr_unary() bug for me (input is big int, ret value is negative), I try to fix it.
On Thu, 08/01 07:52, Eric Blake wrote: > On 08/01/2013 12:31 AM, Fam Zheng wrote: > > Fix it by calling strtoll instead, which will report ERANGE as expected. > > > > (HMP) block_set_io_throttle ide0-hd0 999999999999999999 0 0 0 0 0 > > (HMP) block_set_io_throttle ide0-hd0 9999999999999999999 0 0 0 0 0 > > number too large > > (HMP) block_set_io_throttle ide0-hd0 99999999999999999999 0 0 0 0 0 > > number too large > > Your change causes this error message: > (HMP) block_set_io_throttle ide0-hd0 -99999999999999999999 0 0 0 0 0 > number too large > > Does the "too large" mean in magnitude (correct message) or in value > (misleading message, as any negative number is smaller in value than our > minimum of 0)? OK, it's another thing. If you try this w/o my patch: (qemu) block_set_io_throttle ide0-hd0 -999999999999999999 0 0 0 0 0 bps and iops values must be 0 or greater (qemu) block_set_io_throttle ide0-hd0 -9999999999999999999 0 0 0 0 0 /* Oops, no fail here? Of course it's because int64_t overflow (a * negative negative) . */ (qemu) block_set_io_throttle ide0-hd0 -99999999999999999999 0 0 0 0 0 number too large Because in expr_unary(): 3233 case '-': 3234 next(); 3235 n = -expr_unary(mon); 3236 break; Then you know why, the nested expr_unary(mon) getting absolute part reports too large... > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > monitor.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/monitor.c b/monitor.c > > index 5dc0aa9..7bfb469 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -3286,7 +3286,7 @@ static int64_t expr_unary(Monitor *mon) > > break; > > default: > > errno = 0; > > - n = strtoull(pch, &p, 0); > > + n = strtoll(pch, &p, 0); > > I'm worried that this will break callers that treat their argument as > unsigned, and where the full range of unsigned input was desirable. At > this point, it's probably safer to do a case-by-case analysis of all > callers that use expr_unary() to decide which callers must reject > negative values, instead of making the parser reject numbers that it > previously accepted, thus changing the behavior of callers that treated > the result as unsigned. > You are right, there are callers cast it back to uint64_t, e.g. hmp.c:735 uint32_t size = qdict_get_int(qdict, "size") which means they could get number as large as 9999999999999999999. This is tricky.
diff --git a/monitor.c b/monitor.c index 5dc0aa9..7bfb469 100644 --- a/monitor.c +++ b/monitor.c @@ -3286,7 +3286,7 @@ static int64_t expr_unary(Monitor *mon) break; default: errno = 0; - n = strtoull(pch, &p, 0); + n = strtoll(pch, &p, 0); if (errno == ERANGE) { expr_error(mon, "number too large"); }
We call strtoull(3) to parse a string to int. the range we can accept with our local variable "int64_t n" is (-9223372036854775808 ~ 9223372036854775807), but strtoull(3) can return (0 ~ 18446744073709551615UL). So when we pass a int from HMP within the range of 9223372036854775808 ~ 18446744073709551615, it's safely converted and implicitly casted to int64_t, so n is assigned a negative value, silently, which is incorrect. We can verify this by (HMP) block_set_io_throttle ide0-hd0 999999999999999999 0 0 0 0 0 (HMP) block_set_io_throttle ide0-hd0 9999999999999999999 0 0 0 0 0 bps and iops values must be 0 or greater (HMP) block_set_io_throttle ide0-hd0 99999999999999999999 0 0 0 0 0 number too large The first command succeeds, the second is reporting a negative value error, the third command has correct prompt. Fix it by calling strtoll instead, which will report ERANGE as expected. (HMP) block_set_io_throttle ide0-hd0 999999999999999999 0 0 0 0 0 (HMP) block_set_io_throttle ide0-hd0 9999999999999999999 0 0 0 0 0 number too large (HMP) block_set_io_throttle ide0-hd0 99999999999999999999 0 0 0 0 0 number too large Signed-off-by: Fam Zheng <famz@redhat.com> --- monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)