[Qemu-devel] [PATCH] monitor: fix parsing of big int

[Qemu-devel] [PATCH] monitor: fix parsing of big int

[Fam Zheng]

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

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

Re: [Qemu-devel] [PATCH] monitor: fix parsing of big int

[Eric Blake]

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

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH] monitor: fix parsing of big int

[Luiz Capitulino]

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?

Re: [Qemu-devel] [PATCH] monitor: fix parsing of big int

[Fam Zheng]

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

Re: [Qemu-devel] [PATCH] monitor: fix parsing of big int

[Fam Zheng]

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.

-- 
Fam