[Qemu-devel] [PATCH 1/1] balloon: Ignore negative balloon values

[Qemu-devel] [PATCH 1/1] balloon: Ignore negative balloon values

[Amit Shah]

Negative balloon values don't make sense, ignore them.

Reported-by: Mike Cao <bcao@redhat.com>
Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
I'm not sure if error_report is the right thing to use or should a new
qerror_report() be used.  Luiz, comments?

 balloon.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/balloon.c b/balloon.c
index cf9e3b2..e0ff97f 100644
--- a/balloon.c
+++ b/balloon.c
@@ -51,12 +51,16 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
     return 0;
 }
 
-static int qemu_balloon(ram_addr_t target)
+static int qemu_balloon(long long target)
 {
     if (!balloon_event_fn) {
         return 0;
     }
     trace_balloon_event(balloon_opaque, target);
+    if (target < 0) {
+        error_report("Ignoring negative balloon value");
+        return -1;
+    }
     balloon_event_fn(balloon_opaque, target);
     return 1;
 }
@@ -150,6 +154,8 @@ int do_balloon(Monitor *mon, const QDict *params,
     if (ret == 0) {
         qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
         return -1;
+    } else if (ret < 0) {
+        return -1;
     }
 
     cb(opaque, NULL);
-- 
1.7.6

Re: [Qemu-devel] [PATCH 1/1] balloon: Ignore negative balloon values

[Markus Armbruster]

Amit Shah <amit.shah@redhat.com> writes:

> Negative balloon values don't make sense, ignore them.
>
> Reported-by: Mike Cao <bcao@redhat.com>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
> ---
> I'm not sure if error_report is the right thing to use or should a new
> qerror_report() be used.  Luiz, comments?

Since do_balloon() has been converted to qerror already, you should use
qerror_report().  Something like this should do[*]:

    qerror_report(QERR_INVALID_PARAMETER_VALUE, "target", "a size")

>  balloon.c |    8 +++++++-
>  1 files changed, 7 insertions(+), 1 deletions(-)
>
> diff --git a/balloon.c b/balloon.c
> index cf9e3b2..e0ff97f 100644
> --- a/balloon.c
> +++ b/balloon.c
> @@ -51,12 +51,16 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
>      return 0;
>  }
>  
> -static int qemu_balloon(ram_addr_t target)
> +static int qemu_balloon(long long target)
>  {
>      if (!balloon_event_fn) {
>          return 0;
>      }
>      trace_balloon_event(balloon_opaque, target);
> +    if (target < 0) {
> +        error_report("Ignoring negative balloon value");
> +        return -1;
> +    }
>      balloon_event_fn(balloon_opaque, target);
>      return 1;
>  }

Monitor argument type is 'M', i.e. target_long.  Caller do_balloon() it
as int64_t.  Argument passing casts it to ram_addr_t, which is unsigned.
Negative arguments get misinterpreted.

You fix it by converting to long long instead, then rejecting negative
arguments.

I think do_balloon() is a more natural place to check the argument
range.  Permits keeping qemu_balloon()'s parameter type as is.

> @@ -150,6 +154,8 @@ int do_balloon(Monitor *mon, const QDict *params,
>      if (ret == 0) {
>          qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
>          return -1;
> +    } else if (ret < 0) {
> +        return -1;
>      }
>  
>      cb(opaque, NULL);

[*] Yes, that results in a sub-par error message for humans.  Human
users are advised to appreciate that the error message was created with
proper object-oriented techniques.

Re: [Qemu-devel] [PATCH 1/1] balloon: Ignore negative balloon values

[Amit Shah]

On (Wed) 27 Jul 2011 [15:49:18], Markus Armbruster wrote:
> Amit Shah <amit.shah@redhat.com> writes:
> 
> > Negative balloon values don't make sense, ignore them.
> >
> > Reported-by: Mike Cao <bcao@redhat.com>
> > Signed-off-by: Amit Shah <amit.shah@redhat.com>
> > ---
> > I'm not sure if error_report is the right thing to use or should a new
> > qerror_report() be used.  Luiz, comments?
> 
> Since do_balloon() has been converted to qerror already, you should use
> qerror_report().  Something like this should do[*]:
> 
>     qerror_report(QERR_INVALID_PARAMETER_VALUE, "target", "a size")
> 
> >  balloon.c |    8 +++++++-
> >  1 files changed, 7 insertions(+), 1 deletions(-)
> >
> > diff --git a/balloon.c b/balloon.c
> > index cf9e3b2..e0ff97f 100644
> > --- a/balloon.c
> > +++ b/balloon.c
> > @@ -51,12 +51,16 @@ int qemu_add_balloon_handler(QEMUBalloonEvent *event_func,
> >      return 0;
> >  }
> >  
> > -static int qemu_balloon(ram_addr_t target)
> > +static int qemu_balloon(long long target)
> >  {
> >      if (!balloon_event_fn) {
> >          return 0;
> >      }
> >      trace_balloon_event(balloon_opaque, target);
> > +    if (target < 0) {
> > +        error_report("Ignoring negative balloon value");
> > +        return -1;
> > +    }
> >      balloon_event_fn(balloon_opaque, target);
> >      return 1;
> >  }
> 
> Monitor argument type is 'M', i.e. target_long.  Caller do_balloon() it
> as int64_t.  Argument passing casts it to ram_addr_t, which is unsigned.
> Negative arguments get misinterpreted.
> 
> You fix it by converting to long long instead, then rejecting negative
> arguments.

Ouch; that's crazy.  I don't know why I thought qdict_get_int returned
long long..  I meant to use int64_t.

> I think do_balloon() is a more natural place to check the argument
> range.  Permits keeping qemu_balloon()'s parameter type as is.

OK, done.

> > @@ -150,6 +154,8 @@ int do_balloon(Monitor *mon, const QDict *params,
> >      if (ret == 0) {
> >          qerror_report(QERR_DEVICE_NOT_ACTIVE, "balloon");
> >          return -1;
> > +    } else if (ret < 0) {
> > +        return -1;
> >      }
> >  
> >      cb(opaque, NULL);
> 
> [*] Yes, that results in a sub-par error message for humans.  Human
> users are advised to appreciate that the error message was created with
> proper object-oriented techniques.

Heh.

		Amit