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

[Peter Xu <peterx@redhat.com>]

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