Re: [Qemu-devel] [PATCH 1/2] hmp: expr_unary(): check for overflow in strtoul()

[Eric Blake]

On 04/26/2012 03:10 PM, Luiz Capitulino wrote:
> 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 strtout() overflowed.
> 
> Signed-off-by: Luiz Capitulino <address@hidden>
> ---
>  monitor.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/monitor.c b/monitor.c
> index 8946a10..6178f48 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -3122,8 +3122,14 @@ static int64_t expr_unary(Monitor *mon)
>      default:
>  #if TARGET_PHYS_ADDR_BITS > 32
>          n = strtoull(pch, &p, 0);

strtoull and friends are evil.  That's why libvirt has a rule that all
string-to-integer conversion must go through a libvirt wrapper call, and
_only_ the wrapper may use strtoull (since that one use has been audited
for correctness).

> +        if (n == ULLONG_MAX && errno == ERANGE) {

Not quite right.  ULLONG_MAX is a valid return, but you did not prime
errno, so if errno has junk ERANGE from some earlier point in the
program, you will have a false negative.  You are guaranteed that errno
is unchanged on success, so prime things by setting errno to 0 before
calling strtoull.

> +            expr_error(mon, "error: too long number");

Grammar - this reads better as 'number too long'.

> +        }
>  #else
>          n = strtoul(pch, &p, 0);
> +        if (n == ULONG_MAX && errno == ERANGE) {
> +            expr_error(mon, "error: too long number");
> +        }

Same bug, repeated.

>  #endif
>          if (pch == p) {
>              expr_error(mon, "invalid char in expression");

-- 
Eric Blake   address@hidden    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature