| Message ID | 1335483461-31838-2-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
Luiz Capitulino <lcapitulino@redhat.com> writes: > It's not checked currently, so something like: > > (qemu) balloon -100000000000001111114334234 > (qemu) > > Will just "work" (in this case the balloon command will get a random > value). > > Fix it by checking if strtoul()/strtoull() overflowed. > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > monitor.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/monitor.c b/monitor.c > index 8946a10..56ee971 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -3120,10 +3120,17 @@ static int64_t expr_unary(Monitor *mon) > n = 0; > break; > default: > + errno = 0; > #if TARGET_PHYS_ADDR_BITS > 32 > n = strtoull(pch, &p, 0); > + if (n == ULLONG_MAX && errno == ERANGE) { > + expr_error(mon, "number too large"); > + } The test n == ULLONG_MAX is redundant. You silently interpret a string that doesn't parse as zero. Failure modes of strtol() & friends: 1. String is empty or doesn't have the expected form Test: p == pch 2. String has the expected form, but number overflows result Test: errno == ERANGE (need to errno = 0 before the call) 3. String has the expected form, but there's junk after the number Test: p != pch && "*pch isn't in the follow set" Many places in QEMU silently ignore trailing junk, so not checking this could be excused. > #else > n = strtoul(pch, &p, 0); > + if (n == ULONG_MAX && errno == ERANGE) { > + expr_error(mon, "number too large"); > + } > #endif > if (pch == p) { > expr_error(mon, "invalid char in expression");
On 04/27/2012 03:04 AM, Markus Armbruster wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > >> It's not checked currently, so something like: >> >> +++ b/monitor.c >> @@ -3120,10 +3120,17 @@ static int64_t expr_unary(Monitor *mon) >> n = 0; >> break; >> default: >> + errno = 0; >> #if TARGET_PHYS_ADDR_BITS > 32 >> n = strtoull(pch, &p, 0); >> + if (n == ULLONG_MAX && errno == ERANGE) { >> + expr_error(mon, "number too large"); >> + } > > The test n == ULLONG_MAX is redundant. But harmless. > > You silently interpret a string that doesn't parse as zero. No... >> + if (n == ULONG_MAX && errno == ERANGE) { >> + expr_error(mon, "number too large"); >> + } >> #endif >> if (pch == p) { >> expr_error(mon, "invalid char in expression"); that check was already present.
On Fri, 27 Apr 2012 06:22:48 -0600 Eric Blake <eblake@redhat.com> wrote: > On 04/27/2012 03:04 AM, Markus Armbruster wrote: > > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > >> It's not checked currently, so something like: > >> > > >> +++ b/monitor.c > >> @@ -3120,10 +3120,17 @@ static int64_t expr_unary(Monitor *mon) > >> n = 0; > >> break; > >> default: > >> + errno = 0; > >> #if TARGET_PHYS_ADDR_BITS > 32 > >> n = strtoull(pch, &p, 0); > >> + if (n == ULLONG_MAX && errno == ERANGE) { > >> + expr_error(mon, "number too large"); > >> + } > > > > The test n == ULLONG_MAX is redundant. > > But harmless. Yes, and I prefer to comply to the standard (as there's no strong reason not to do so). Btw, I plan to add a wrapper like libvirt and convert qemu users.
Eric Blake <eblake@redhat.com> writes: > On 04/27/2012 03:04 AM, Markus Armbruster wrote: >> Luiz Capitulino <lcapitulino@redhat.com> writes: [...] >> You silently interpret a string that doesn't parse as zero. > > No... > >>> + if (n == ULONG_MAX && errno == ERANGE) { >>> + expr_error(mon, "number too large"); >>> + } >>> #endif >>> if (pch == p) { >>> expr_error(mon, "invalid char in expression"); > > that check was already present. You mean I'm supposed to read patch context?
Luiz Capitulino <lcapitulino@redhat.com> writes: > On Fri, 27 Apr 2012 06:22:48 -0600 > Eric Blake <eblake@redhat.com> wrote: > >> On 04/27/2012 03:04 AM, Markus Armbruster wrote: >> > Luiz Capitulino <lcapitulino@redhat.com> writes: >> > >> >> It's not checked currently, so something like: >> >> >> >> >> +++ b/monitor.c >> >> @@ -3120,10 +3120,17 @@ static int64_t expr_unary(Monitor *mon) >> >> n = 0; >> >> break; >> >> default: >> >> + errno = 0; >> >> #if TARGET_PHYS_ADDR_BITS > 32 >> >> n = strtoull(pch, &p, 0); >> >> + if (n == ULLONG_MAX && errno == ERANGE) { >> >> + expr_error(mon, "number too large"); >> >> + } >> > >> > The test n == ULLONG_MAX is redundant. >> >> But harmless. > > Yes, and I prefer to comply to the standard (as there's no strong > reason not to do so). What standard? Checking errno is necessary and sufficient for detecting overflow. [...]
On Fri, 27 Apr 2012 15:24:51 +0200 Markus Armbruster <armbru@redhat.com> wrote: > Luiz Capitulino <lcapitulino@redhat.com> writes: > > > On Fri, 27 Apr 2012 06:22:48 -0600 > > Eric Blake <eblake@redhat.com> wrote: > > > >> On 04/27/2012 03:04 AM, Markus Armbruster wrote: > >> > Luiz Capitulino <lcapitulino@redhat.com> writes: > >> > > >> >> It's not checked currently, so something like: > >> >> > >> > >> >> +++ b/monitor.c > >> >> @@ -3120,10 +3120,17 @@ static int64_t expr_unary(Monitor *mon) > >> >> n = 0; > >> >> break; > >> >> default: > >> >> + errno = 0; > >> >> #if TARGET_PHYS_ADDR_BITS > 32 > >> >> n = strtoull(pch, &p, 0); > >> >> + if (n == ULLONG_MAX && errno == ERANGE) { > >> >> + expr_error(mon, "number too large"); > >> >> + } > >> > > >> > The test n == ULLONG_MAX is redundant. > >> > >> But harmless. > > > > Yes, and I prefer to comply to the standard (as there's no strong > > reason not to do so). > > What standard? http://pubs.opengroup.org/onlinepubs/009695399/functions/strtoul.html
On 04/27/2012 07:26 AM, Luiz Capitulino wrote: >>>>> The test n == ULLONG_MAX is redundant. >>>> >>>> But harmless. >>> >>> Yes, and I prefer to comply to the standard (as there's no strong >>> reason not to do so). >> >> What standard? > > http://pubs.opengroup.org/onlinepubs/009695399/functions/strtoul.html Which says that ERANGE is only possible if the return is ULLONG_MAX, but since it also says that errno is unchanged on success, and that errno must be ERANGE on overflow, it is sufficient to check errno==ERANGE without having to also check the return value as a way to detect overflow. The only possible difference it might make to check the return value first is that you can filter out an errno check for all other return values, and on platforms where 'errno' evaluates to a macro that invokes a function in order to access a thread-local, then avoiding an errno comparison on the common case of no overflow is a potentially worthwhile micro-optimization. Again, all this conversation just goes to strengthen my claim that the strtol() UI is evil, and that the only decent way to do string-to-integer processing is to wrap it in a nicer interface, so that it becomes easier to audit just the wrapper instead of all callers for compliance with all the subtle details.
On Fri, 27 Apr 2012 09:22:46 -0600 Eric Blake <eblake@redhat.com> wrote: > On 04/27/2012 07:26 AM, Luiz Capitulino wrote: > >>>>> The test n == ULLONG_MAX is redundant. > >>>> > >>>> But harmless. > >>> > >>> Yes, and I prefer to comply to the standard (as there's no strong > >>> reason not to do so). > >> > >> What standard? > > > > http://pubs.opengroup.org/onlinepubs/009695399/functions/strtoul.html > > Which says that ERANGE is only possible if the return is ULLONG_MAX, but > since it also says that errno is unchanged on success, and that errno > must be ERANGE on overflow, it is sufficient to check errno==ERANGE > without having to also check the return value as a way to detect > overflow. The only possible difference it might make to check the > return value first is that you can filter out an errno check for all > other return values, and on platforms where 'errno' evaluates to a macro > that invokes a function in order to access a thread-local, then avoiding > an errno comparison on the common case of no overflow is a potentially > worthwhile micro-optimization. > > Again, all this conversation just goes to strengthen my claim that the > strtol() UI is evil, and that the only decent way to do > string-to-integer processing is to wrap it in a nicer interface, so that > it becomes easier to audit just the wrapper instead of all callers for > compliance with all the subtle details. I plan to introduce the wrapper in the near future.
diff --git a/monitor.c b/monitor.c index 8946a10..56ee971 100644 --- a/monitor.c +++ b/monitor.c @@ -3120,10 +3120,17 @@ static int64_t expr_unary(Monitor *mon) n = 0; break; default: + errno = 0; #if TARGET_PHYS_ADDR_BITS > 32 n = strtoull(pch, &p, 0); + if (n == ULLONG_MAX && errno == ERANGE) { + expr_error(mon, "number too large"); + } #else n = strtoul(pch, &p, 0); + if (n == ULONG_MAX && errno == ERANGE) { + expr_error(mon, "number too large"); + } #endif if (pch == p) { expr_error(mon, "invalid char in expression");
It's not checked currently, so something like: (qemu) balloon -100000000000001111114334234 (qemu) Will just "work" (in this case the balloon command will get a random value). Fix it by checking if strtoul()/strtoull() overflowed. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- monitor.c | 7 +++++++ 1 file changed, 7 insertions(+)