Re: [Qemu-devel] [PATCH] monitor: Fix Resource leak

[Gonglei <arei.gonglei@huawei.com>]

On 2015/2/11 21:00, Markus Armbruster wrote:
> <arei.gonglei@huawei.com> writes:
> 
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> Coverity spot:
>> qemu_using_spice allocates memory that is stored into err,
>> but not freed before return.
>>
>> Cc:Markus Armbruster <armbru@redhat.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> ---
>>  monitor.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/monitor.c b/monitor.c
>> index c3cc060..137d23f 100644
>> --- a/monitor.c
>> +++ b/monitor.c
>> @@ -1095,12 +1095,13 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
>>      const char *subject  = qdict_get_try_str(qdict, "cert-subject");
>>      int port             = qdict_get_try_int(qdict, "port", -1);
>>      int tls_port         = qdict_get_try_int(qdict, "tls-port", -1);
>> -    Error *err;
>> +    Error *err = NULL;
>>      int ret;
>>  
>>      if (strcmp(protocol, "spice") == 0) {
>>          if (!qemu_using_spice(&err)) {
>>              qerror_report_err(err);
>> +            error_free(err);
>>              return -1;
>>          }
> 
> Your commit message is incomplete.  The resource leak is real, but it's
> the less serious bug here.  The serious one is missing initialization of
> err.
> 
> If using_spice, qemu_using_spice() returns true without touching err.
> All fine.
> 
> Else, if err is null by chance, qemu_using_spice()'s error_set() creates
> an error object and stores it in err.  We leak it.
> 
> If err is not null, error_set() fails its assertion.
> 
Thanks for your explanation, I haven't a global inspection TBH.
> Broken in commit b25d81b by yours truly.  Thanks for cleaning up my
> mess!
> 
> Let's fix up the commit message:
> 
>     monitor: Fix missing err = NULL in client_migrate_info()
> 
>     When SPICE isn't used, we either fail an assertion in error_set(),
>     or leak an error object.  Broken in commit b25d81b.
> 
OK, will fix.
> With such a fixup:
> 
> Reviewed-by: Markus Armbruster <armbru@redhat.com>
> 
Thanks.

Regards,
-Gonglei