qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Markus Armbruster
Subject: Re: [Qemu-devel] [PATCH] monitor: fix double-free of request error
Date: Fri, 06 Jul 2018 08:25:57 +0200
User-agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)

Peter Xu <address@hidden> 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 <address@hidden>
>
> Reviewed-by: Peter Xu <address@hidden>
>
> 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.



reply via email to

[Prev in Thread] Current Thread [Next in Thread]