monitor: fix parsing of big int
diff mbox

Message ID 1375338695-670-1-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Aug. 1, 2013, 6:31 a.m. UTC
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(-)

Comments

Eric Blake Aug. 1, 2013, 1:52 p.m. UTC | #1
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.
Luiz Capitulino Aug. 1, 2013, 2 p.m. UTC | #2
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?
Fam Zheng Aug. 2, 2013, 2:39 a.m. UTC | #3
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.
Fam Zheng Aug. 2, 2013, 3:07 a.m. UTC | #4
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.

Patch
diff mbox

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