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

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

[arei.gonglei]

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;
         }
 
-- 
1.7.12.4

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

[Markus Armbruster]

<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.

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.

With such a fixup:

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

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

[Gonglei]

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

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

[arei.gonglei]

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;
         }
 
-- 
1.7.12.4