[Qemu-devel] [PATCH v2] spice: fix memory leak

[Qemu-devel] [PATCH v2] spice: fix memory leak

[arei.gonglei]

From: Gonglei <arei.gonglei@huawei.com>

If errors happen for middle items of channel_list,
qmp_query_spice_channels() returns NULL, and the variable
cur_item going out of scope leaks the storage it points to.

The flag is a compatibility thing for older spice-server
versions. Meanwhile our minimum spice version requirement is
new enough that we should never ever see this error, and if we
do something went very seriously wrong. Let's using assert()
instead of returning NULL to avoid a memory leak.

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Gonglei <arei.gonglei@huawei.com>
Reviewed-by: Eric Blake <eblake@redhat.com>
---
v2: 
 - using assert() instead of returning NULL. (Gerd)
 - fix some typos. (Eric)
 - add Eric's R-by tag, hope Eric has no objection
   because of changes of v2. :)
---
 ui/spice-core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/ui/spice-core.c b/ui/spice-core.c
index 6467fa4..1100f8e 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -386,10 +386,7 @@ static SpiceChannelList *qmp_query_spice_channels(void)
         struct sockaddr *paddr;
         socklen_t plen;
 
-        if (!(item->info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT)) {
-            error_report("invalid channel event");
-            return NULL;
-        }
+        assert(item->info->flags & SPICE_CHANNEL_EVENT_FLAG_ADDR_EXT);
 
         chan = g_malloc0(sizeof(*chan));
         chan->value = g_malloc0(sizeof(*chan->value));
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH v2] spice: fix memory leak

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1348 bytes --]

On 12/05/2014 01:30 AM, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> If errors happen for middle items of channel_list,
> qmp_query_spice_channels() returns NULL, and the variable
> cur_item going out of scope leaks the storage it points to.
> 
> The flag is a compatibility thing for older spice-server
> versions. Meanwhile our minimum spice version requirement is
> new enough that we should never ever see this error, and if we
> do something went very seriously wrong. Let's using assert()
> instead of returning NULL to avoid a memory leak.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Putting in a R-b at the same time as fundamental changes (going from two
loops to an assert) is not something I would have done if submitting the
patch...

> ---
> v2: 
>  - using assert() instead of returning NULL. (Gerd)
>  - fix some typos. (Eric)
>  - add Eric's R-by tag, hope Eric has no objection
>    because of changes of v2. :)

...but at least you caught my attention, and I am okay with v2, so you
can keep it now.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 539 bytes --]

Re: [Qemu-devel] [PATCH v2] spice: fix memory leak

[Gonglei]

On 2014/12/5 20:56, Eric Blake wrote:

> On 12/05/2014 01:30 AM, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> If errors happen for middle items of channel_list,
>> qmp_query_spice_channels() returns NULL, and the variable
>> cur_item going out of scope leaks the storage it points to.
>>
>> The flag is a compatibility thing for older spice-server
>> versions. Meanwhile our minimum spice version requirement is
>> new enough that we should never ever see this error, and if we
>> do something went very seriously wrong. Let's using assert()
>> instead of returning NULL to avoid a memory leak.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> 
> Putting in a R-b at the same time as fundamental changes (going from two
> loops to an assert) is not something I would have done if submitting the
> patch...
> 

Yes, it's my fault :(

>> ---
>> v2: 
>>  - using assert() instead of returning NULL. (Gerd)
>>  - fix some typos. (Eric)
>>  - add Eric's R-by tag, hope Eric has no objection
>>    because of changes of v2. :)
> 
> ...but at least you caught my attention, and I am okay with v2, so you
> can keep it now.
> 

> Reviewed-by: Eric Blake <eblake@redhat.com>
> 

Thanks.

Regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2] spice: fix memory leak

[Gerd Hoffmann]

On Fr, 2014-12-05 at 16:30 +0800, arei.gonglei@huawei.com wrote:
> From: Gonglei <arei.gonglei@huawei.com>
> 
> If errors happen for middle items of channel_list,
> qmp_query_spice_channels() returns NULL, and the variable
> cur_item going out of scope leaks the storage it points to.
> 
> The flag is a compatibility thing for older spice-server
> versions. Meanwhile our minimum spice version requirement is
> new enough that we should never ever see this error, and if we
> do something went very seriously wrong. Let's using assert()
> instead of returning NULL to avoid a memory leak.
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>

Added to spice patch queue.

thanks
  Gerd

Re: [Qemu-devel] [PATCH v2] spice: fix memory leak

[Stefan Weil]

Am 08.12.2014 um 10:25 schrieb Gerd Hoffmann:
> On Fr, 2014-12-05 at 16:30 +0800, arei.gonglei@huawei.com wrote:
>> From: Gonglei <arei.gonglei@huawei.com>
>>
>> If errors happen for middle items of channel_list,
>> qmp_query_spice_channels() returns NULL, and the variable
>> cur_item going out of scope leaks the storage it points to.
>>
>> The flag is a compatibility thing for older spice-server
>> versions. Meanwhile our minimum spice version requirement is
>> new enough that we should never ever see this error, and if we
>> do something went very seriously wrong. Let's using assert()
>> instead of returning NULL to avoid a memory leak.
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
>> Reviewed-by: Eric Blake <eblake@redhat.com>
> Added to spice patch queue.
>
> thanks
>   Gerd


Shouldn't we use g_assert instead of assert? Maybe you can fix this
without a new patch iteration.

Stefan

Re: [Qemu-devel] [PATCH v2] spice: fix memory leak

[Gerd Hoffmann]

On Mo, 2014-12-08 at 10:31 +0100, Stefan Weil wrote:
> Am 08.12.2014 um 10:25 schrieb Gerd Hoffmann:
> > On Fr, 2014-12-05 at 16:30 +0800, arei.gonglei@huawei.com wrote:
> >> From: Gonglei <arei.gonglei@huawei.com>
> >>
> >> If errors happen for middle items of channel_list,
> >> qmp_query_spice_channels() returns NULL, and the variable
> >> cur_item going out of scope leaks the storage it points to.
> >>
> >> The flag is a compatibility thing for older spice-server
> >> versions. Meanwhile our minimum spice version requirement is
> >> new enough that we should never ever see this error, and if we
> >> do something went very seriously wrong. Let's using assert()
> >> instead of returning NULL to avoid a memory leak.
> >>
> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com>
> >> Reviewed-by: Eric Blake <eblake@redhat.com>
> > Added to spice patch queue.
> >
> > thanks
> >   Gerd
> 
> 
> Shouldn't we use g_assert instead of assert?

Why?  I rarely see g_assert in the tree, with the exception of tests/

cheers,
  Gerd

[Qemu-devel] [PATCH v2] spice: fix memory leak

[Gerd Hoffmann]

Found by coverity.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
---
 monitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/monitor.c b/monitor.c
index c3cc060..2c37953 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1101,6 +1101,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
     if (strcmp(protocol, "spice") == 0) {
         if (!qemu_using_spice(&err)) {
             qerror_report_err(err);
+            error_free(err);
             return -1;
         }
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH v2] spice: fix memory leak

[Gonglei]

On 2015/2/12 0:50, Gerd Hoffmann wrote:
> Found by coverity.
> 
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  monitor.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/monitor.c b/monitor.c
> index c3cc060..2c37953 100644
> --- a/monitor.c
> +++ b/monitor.c
> @@ -1101,6 +1101,7 @@ static int client_migrate_info(Monitor *mon, const QDict *qdict,
>      if (strcmp(protocol, "spice") == 0) {
>          if (!qemu_using_spice(&err)) {
>              qerror_report_err(err);
> +            error_free(err);
>              return -1;
>          }
>  
> 
Hi, Gerd
the err variable should be initialized to NULL.

This leak had been fixed by:
http://patchwork.ozlabs.org/patch/438696/

and v2:
[PATCH v2] monitor: Fix missing err = NULL in client_migrate_info()

Regards,
-Gonglei