* [Qemu-devel] [PATCH] monitor: fix double-free of request error @ 2018-07-05 16:42 Marc-André Lureau 2018-07-06 4:06 ` Peter Xu 2018-07-06 6:00 ` Markus Armbruster 0 siblings, 2 replies; 5+ messages in thread From: Marc-André Lureau @ 2018-07-05 16:42 UTC (permalink / raw) To: qemu-devel; +Cc: armbru, eblake, 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error 2018-07-05 16:42 [Qemu-devel] [PATCH] monitor: fix double-free of request error Marc-André Lureau @ 2018-07-06 4:06 ` Peter Xu 2018-07-06 6:25 ` Markus Armbruster 2018-07-06 6:00 ` Markus Armbruster 1 sibling, 1 reply; 5+ messages in thread From: Peter Xu @ 2018-07-06 4:06 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel, armbru 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error 2018-07-06 4:06 ` Peter Xu @ 2018-07-06 6:25 ` Markus Armbruster 2018-07-06 7:33 ` Peter Xu 0 siblings, 1 reply; 5+ messages in thread From: Markus Armbruster @ 2018-07-06 6:25 UTC (permalink / raw) To: Peter Xu; +Cc: Marc-André Lureau, qemu-devel, armbru 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error 2018-07-06 6:25 ` Markus Armbruster @ 2018-07-06 7:33 ` Peter Xu 0 siblings, 0 replies; 5+ messages in thread From: Peter Xu @ 2018-07-06 7:33 UTC (permalink / raw) To: Markus Armbruster; +Cc: Marc-André Lureau, qemu-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error 2018-07-05 16:42 [Qemu-devel] [PATCH] monitor: fix double-free of request error Marc-André Lureau 2018-07-06 4:06 ` Peter Xu @ 2018-07-06 6:00 ` Markus Armbruster 1 sibling, 0 replies; 5+ messages in thread From: Markus Armbruster @ 2018-07-06 6:00 UTC (permalink / raw) To: Marc-André Lureau; +Cc: qemu-devel 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> ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-07-06 7:33 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-07-05 16:42 [Qemu-devel] [PATCH] monitor: fix double-free of request error Marc-André Lureau 2018-07-06 4:06 ` Peter Xu 2018-07-06 6:25 ` Markus Armbruster 2018-07-06 7:33 ` Peter Xu 2018-07-06 6:00 ` Markus Armbruster
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).