[Qemu-devel] [PATCH] monitor: fix double-free of request error

[Qemu-devel] [PATCH] monitor: fix double-free of request error

[Marc-André Lureau]

qmp_error_response() will free the given error. Fix double-free in
later qmp_request_free().

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 monitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/monitor.c b/monitor.c
index 3c9c97b73f..7af1f18d13 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4186,6 +4186,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
     } else {
         assert(req_obj->err);
         rsp = qmp_error_response(req_obj->err);
+        req_obj->err = NULL;
         monitor_qmp_respond(req_obj->mon, rsp, NULL);
         qobject_unref(rsp);
     }
-- 
2.18.0.rc1

Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error

[Peter Xu]

On Thu, Jul 05, 2018 at 06:42:01PM +0200, Marc-André Lureau wrote:
> qmp_error_response() will free the given error. Fix double-free in
> later qmp_request_free().
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

And not related to current patch...

> ---
>  monitor.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/monitor.c b/monitor.c
> index 3c9c97b73f..7af1f18d13 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4186,6 +4186,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>      } else {
>          assert(req_obj->err);
>          rsp = qmp_error_response(req_obj->err);
> +        req_obj->err = NULL;
>          monitor_qmp_respond(req_obj->mon, rsp, NULL);

... here not sure whether we should just pass in req_obj->id instead
of NULL, or maybe we can do some more assertions like:

diff --git a/monitor.c b/monitor.c
index 9eb9f06599..04d2c50f4e 100644
--- a/monitor.c
+++ b/monitor.c
@@ -4215,10 +4215,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
 
     mon = req_obj->mon;
     if (req_obj->req) {
+        assert(!req_obj->err);
         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
         monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
     } else {
         assert(req_obj->err);
+        assert(!req_obj->id);
         rsp = qmp_error_response(req_obj->err);
         monitor_qmp_respond(mon, rsp, NULL);
         qobject_unref(rsp);

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error

[Markus Armbruster]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> qmp_error_response() will free the given error. Fix double-free in
> later qmp_request_free().
>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

Broken in commit 1cc37471525 by yours truly.  I'll add that to the
commit message when I apply.

Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> On Thu, Jul 05, 2018 at 06:42:01PM +0200, Marc-André Lureau wrote:
>> qmp_error_response() will free the given error. Fix double-free in
>> later qmp_request_free().
>> 
>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>
> Reviewed-by: Peter Xu <peterx@redhat.com>
>
> And not related to current patch...
>
>> ---
>>  monitor.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/monitor.c b/monitor.c
>> index 3c9c97b73f..7af1f18d13 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -4186,6 +4186,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
>>      } else {
>>          assert(req_obj->err);
>>          rsp = qmp_error_response(req_obj->err);
>> +        req_obj->err = NULL;
>>          monitor_qmp_respond(req_obj->mon, rsp, NULL);
>
> ... here not sure whether we should just pass in req_obj->id instead
> of NULL, or maybe we can do some more assertions like:
>
> diff --git a/monitor.c b/monitor.c
> index 9eb9f06599..04d2c50f4e 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -4215,10 +4215,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
>  
>      mon = req_obj->mon;
>      if (req_obj->req) {
> +        assert(!req_obj->err);

Makes sense.

>          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
>          monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
>      } else {
>          assert(req_obj->err);
> +        assert(!req_obj->id);

I'd simply pass req_obj->id to monitor_qmp_respond().  Yes, it'll always
be null, but the code would do the right thing if that should ever
change.

>          rsp = qmp_error_response(req_obj->err);
>          monitor_qmp_respond(mon, rsp, NULL);
>          qobject_unref(rsp);
>
> Thanks,

Perhaps even reorder to put the error case first:

    if (req_obj->err) {
        assert(!req_obj->req);
        rsp = qmp_error_response(req_obj->err);
        req_obj->err = NULL;
        monitor_qmp_respond(req_obj->mon, rsp, req_obj->id);
        qobject_unref(rsp);
    } else if (req_obj->req) {
        trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
        monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
    }

Matter of taste.

Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error

[Peter Xu]

On Fri, Jul 06, 2018 at 08:25:57AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > On Thu, Jul 05, 2018 at 06:42:01PM +0200, Marc-André Lureau wrote:
> >> qmp_error_response() will free the given error. Fix double-free in
> >> later qmp_request_free().
> >> 
> >> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> >
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> >
> > And not related to current patch...
> >
> >> ---
> >>  monitor.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >> 
> >> diff --git a/monitor.c b/monitor.c
> >> index 3c9c97b73f..7af1f18d13 100644
> >> --- a/monitor.c
> >> +++ b/monitor.c
> >> @@ -4186,6 +4186,7 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >>      } else {
> >>          assert(req_obj->err);
> >>          rsp = qmp_error_response(req_obj->err);
> >> +        req_obj->err = NULL;
> >>          monitor_qmp_respond(req_obj->mon, rsp, NULL);
> >
> > ... here not sure whether we should just pass in req_obj->id instead
> > of NULL, or maybe we can do some more assertions like:
> >
> > diff --git a/monitor.c b/monitor.c
> > index 9eb9f06599..04d2c50f4e 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -4215,10 +4215,12 @@ static void monitor_qmp_bh_dispatcher(void *data)
> >  
> >      mon = req_obj->mon;
> >      if (req_obj->req) {
> > +        assert(!req_obj->err);
> 
> Makes sense.
> 
> >          trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
> >          monitor_qmp_dispatch(mon, req_obj->req, req_obj->id);
> >      } else {
> >          assert(req_obj->err);
> > +        assert(!req_obj->id);
> 
> I'd simply pass req_obj->id to monitor_qmp_respond().  Yes, it'll always
> be null, but the code would do the right thing if that should ever
> change.

Agreed.

> 
> >          rsp = qmp_error_response(req_obj->err);
> >          monitor_qmp_respond(mon, rsp, NULL);
> >          qobject_unref(rsp);
> >
> > Thanks,
> 
> Perhaps even reorder to put the error case first:
> 
>     if (req_obj->err) {
>         assert(!req_obj->req);
>         rsp = qmp_error_response(req_obj->err);
>         req_obj->err = NULL;
>         monitor_qmp_respond(req_obj->mon, rsp, req_obj->id);
>         qobject_unref(rsp);
>     } else if (req_obj->req) {
>         trace_monitor_qmp_cmd_in_band(qobject_get_try_str(req_obj->id) ?: "");
>         monitor_qmp_dispatch(req_obj->mon, req_obj->req, req_obj->id);
>     }
> 
> Matter of taste.

Looks good to me.  Thanks,

-- 
Peter Xu