| Message ID | 1335474606-22889-2-git-send-email-lcapitulino@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 26 April 2012 22:10, Luiz Capitulino <lcapitulino@redhat.com> 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. "strtoul()/strtoull()". > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > --- > 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); > + if (n == ULLONG_MAX && errno == ERANGE) { > + expr_error(mon, "error: too long number"); > + } > #else > n = strtoul(pch, &p, 0); > + if (n == ULONG_MAX && errno == ERANGE) { > + expr_error(mon, "error: too long number"); > + } > #endif > if (pch == p) { > expr_error(mon, "invalid char in expression"); None of the other expr_error()s here say "error: ", so for consistency this one shouldn't either. Also, perhaps "number out of range" or "number too large" would sound better? (Incidentally I was surprised that this code cared about TARGET_PHYS_ADDR_BITS...) -- PMM
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 <lcapitulino@redhat.com> > --- > 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");
On Thu, 26 Apr 2012 22:30:20 +0100 Peter Maydell <peter.maydell@linaro.org> wrote: > On 26 April 2012 22:10, Luiz Capitulino <lcapitulino@redhat.com> 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. > > "strtoul()/strtoull()". Ah, thanks. > > > Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > 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); > > + if (n == ULLONG_MAX && errno == ERANGE) { > > + expr_error(mon, "error: too long number"); > > + } > > #else > > n = strtoul(pch, &p, 0); > > + if (n == ULONG_MAX && errno == ERANGE) { > > + expr_error(mon, "error: too long number"); > > + } > > #endif > > if (pch == p) { > > expr_error(mon, "invalid char in expression"); > > None of the other expr_error()s here say "error: ", so > for consistency this one shouldn't either. > Also, perhaps "number out of range" or "number too large" > would sound better? Agree, will make the change.
On Thu, 26 Apr 2012 15:34:50 -0600 Eric Blake <eblake@redhat.com> wrote: > 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 <lcapitulino@redhat.com> > > --- > > 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). We could add a wrapper too, but I'll only fix the bug for now. > > > + 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. I thought about doing that but - hope that won't sound stupid - how can I guarantee that ERANGE is not 0? > > + expr_error(mon, "error: too long number"); > > Grammar - this reads better as 'number too long'. Yes, Peter suggested something similar. Will change that.
On Thu, 26 Apr 2012 20:18:02 -0300 Luiz Capitulino <lcapitulino@redhat.com> wrote: > > 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. > > I thought about doing that but - hope that won't sound stupid - how can I > guarantee that ERANGE is not 0? Oh, that _was_ stupid. errno=0 is actually recommended in the documentation, will change.
On Thu, 26 Apr 2012 20:19:42 -0300 Luiz Capitulino <lcapitulino@redhat.com> wrote: > On Thu, 26 Apr 2012 20:18:02 -0300 > Luiz Capitulino <lcapitulino@redhat.com> wrote: > > > > 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. > > > > I thought about doing that but - hope that won't sound stupid - how can I > > guarantee that ERANGE is not 0? > > Oh, that _was_ stupid. errno=0 is actually recommended in the documentation, > will change. Oh, and now I read that you wrote just that above. Dude, it's not a good idea to answer emails while waiting for your food to come...
On 04/26/2012 05:18 PM, Luiz Capitulino wrote: >> 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. > > I thought about doing that but - hope that won't sound stupid - how can I > guarantee that ERANGE is not 0? Because POSIX requires that all E* constants are positive numbers, reserving errno of 0 for success.
On Fri, 27 Apr 2012 08:07:20 -0600 Eric Blake <eblake@redhat.com> wrote: > On 04/26/2012 05:18 PM, Luiz Capitulino wrote: > >> 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. > > > > I thought about doing that but - hope that won't sound stupid - how can I > > guarantee that ERANGE is not 0? > > Because POSIX requires that all E* constants are positive numbers, > reserving errno of 0 for success. Yeah, I read about that a few hours later. But thanks anyway!
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); + if (n == ULLONG_MAX && errno == ERANGE) { + expr_error(mon, "error: too long number"); + } #else n = strtoul(pch, &p, 0); + if (n == ULONG_MAX && errno == ERANGE) { + expr_error(mon, "error: too long number"); + } #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 strtout() overflowed. Signed-off-by: Luiz Capitulino <lcapitulino@redhat.com> --- monitor.c | 6 ++++++ 1 file changed, 6 insertions(+)