[Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Chen Gang]

Call g_free() after cache_fini() in migration_end(), but do not call
g_free() after call cache_fini() in xbzrle_cache_resize() which will
cause memory leak.

cache_init() and cache_fini() are pair, so need let cache_fini() call
g_free(cache) to match cache_init(), then fix current issue too.


Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
---
 arch_init.c  | 1 -
 page_cache.c | 1 +
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch_init.c b/arch_init.c
index 9f1a174..23044c1 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -739,7 +739,6 @@ static void migration_end(void)
     XBZRLE_cache_lock();
     if (XBZRLE.cache) {
         cache_fini(XBZRLE.cache);
-        g_free(XBZRLE.cache);
         g_free(XBZRLE.encoded_buf);
         g_free(XBZRLE.current_buf);
         XBZRLE.cache = NULL;
diff --git a/page_cache.c b/page_cache.c
index b033681..89bb1ec 100644
--- a/page_cache.c
+++ b/page_cache.c
@@ -109,6 +109,7 @@ void cache_fini(PageCache *cache)
 
     g_free(cache->page_cache);
     cache->page_cache = NULL;
+    g_free(cache);
 }
 
 static size_t cache_get_cache_pos(const PageCache *cache,
-- 
1.7.11.7

Re: [Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[ChenLiang]

On 2014/6/2 20:16, Chen Gang wrote:

> Call g_free() after cache_fini() in migration_end(), but do not call
> g_free() after call cache_fini() in xbzrle_cache_resize() which will
> cause memory leak.
> 
> cache_init() and cache_fini() are pair, so need let cache_fini() call
> g_free(cache) to match cache_init(), then fix current issue too.
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>
> ---
>  arch_init.c  | 1 -
>  page_cache.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 9f1a174..23044c1 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -739,7 +739,6 @@ static void migration_end(void)
>      XBZRLE_cache_lock();
>      if (XBZRLE.cache) {
>          cache_fini(XBZRLE.cache);
> -        g_free(XBZRLE.cache);
>          g_free(XBZRLE.encoded_buf);
>          g_free(XBZRLE.current_buf);
>          XBZRLE.cache = NULL;
> diff --git a/page_cache.c b/page_cache.c
> index b033681..89bb1ec 100644
> --- a/page_cache.c
> +++ b/page_cache.c
> @@ -109,6 +109,7 @@ void cache_fini(PageCache *cache)
>  
>      g_free(cache->page_cache);
>      cache->page_cache = NULL;
> +    g_free(cache);
>  }
>  
>  static size_t cache_get_cache_pos(const PageCache *cache,


Reviewed-by: ChenLiang <chenliang88@huawei.com>

Re: [Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> Call g_free() after cache_fini() in migration_end(), but do not call
> g_free() after call cache_fini() in xbzrle_cache_resize() which will
> cause memory leak.
>
> cache_init() and cache_fini() are pair, so need let cache_fini() call
> g_free(cache) to match cache_init(), then fix current issue too.

I'm not sure I get you.  Is the following accurate?

    migration: Plug memory leak in migrate-set-cache-size command

    We call g_free() after cache_fini() in migration_end(), but we don't
    call it after cache_fini() in xbzrle_cache_resize(), leaking the
    memory.

    cache_init() and cache_fini() are a pair.  Since cache_init()
    allocates the cache, let cache_fini() free it.  This plugs the leak.

Re: [Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Chen Gang]

On 06/04/2014 04:16 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> Call g_free() after cache_fini() in migration_end(), but do not call
>> g_free() after call cache_fini() in xbzrle_cache_resize() which will
>> cause memory leak.
>>
>> cache_init() and cache_fini() are pair, so need let cache_fini() call
>> g_free(cache) to match cache_init(), then fix current issue too.
> 
> I'm not sure I get you.  Is the following accurate?
> 
>     migration: Plug memory leak in migrate-set-cache-size command
> 
>     We call g_free() after cache_fini() in migration_end(), but we don't
>     call it after cache_fini() in xbzrle_cache_resize(), leaking the
>     memory.
> 
>     cache_init() and cache_fini() are a pair.  Since cache_init()
>     allocates the cache, let cache_fini() free it.  This plugs the leak.
> 

Yeah, and excuse me for my poor English.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Markus Armbruster]

Chen Gang <gang.chen.5i5j@gmail.com> writes:

> On 06/04/2014 04:16 PM, Markus Armbruster wrote:
>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>> 
>>> Call g_free() after cache_fini() in migration_end(), but do not call
>>> g_free() after call cache_fini() in xbzrle_cache_resize() which will
>>> cause memory leak.
>>>
>>> cache_init() and cache_fini() are pair, so need let cache_fini() call
>>> g_free(cache) to match cache_init(), then fix current issue too.
>> 
>> I'm not sure I get you.  Is the following accurate?
>> 
>>     migration: Plug memory leak in migrate-set-cache-size command
>> 
>>     We call g_free() after cache_fini() in migration_end(), but we don't
>>     call it after cache_fini() in xbzrle_cache_resize(), leaking the
>>     memory.
>> 
>>     cache_init() and cache_fini() are a pair.  Since cache_init()
>>     allocates the cache, let cache_fini() free it.  This plugs the leak.
>> 
>
> Yeah, and excuse me for my poor English.

No problem, we figured it out together :)

Would be nice to use the above text as commit message.  Perhaps it can
be done on commit.

Re: [Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Chen Gang]

On 06/04/2014 06:54 PM, Markus Armbruster wrote:
> Chen Gang <gang.chen.5i5j@gmail.com> writes:
> 
>> On 06/04/2014 04:16 PM, Markus Armbruster wrote:
>>> Chen Gang <gang.chen.5i5j@gmail.com> writes:
>>>
>>>> Call g_free() after cache_fini() in migration_end(), but do not call
>>>> g_free() after call cache_fini() in xbzrle_cache_resize() which will
>>>> cause memory leak.
>>>>
>>>> cache_init() and cache_fini() are pair, so need let cache_fini() call
>>>> g_free(cache) to match cache_init(), then fix current issue too.
>>>
>>> I'm not sure I get you.  Is the following accurate?
>>>
>>>     migration: Plug memory leak in migrate-set-cache-size command
>>>
>>>     We call g_free() after cache_fini() in migration_end(), but we don't
>>>     call it after cache_fini() in xbzrle_cache_resize(), leaking the
>>>     memory.
>>>
>>>     cache_init() and cache_fini() are a pair.  Since cache_init()
>>>     allocates the cache, let cache_fini() free it.  This plugs the leak.
>>>
>>
>> Yeah, and excuse me for my poor English.
> 
> No problem, we figured it out together :)
> 
> Would be nice to use the above text as commit message.  Perhaps it can
> be done on commit.
> 

That is fine to me.


Thanks.
-- 
Chen Gang

Open, share, and attitude like air, water, and life which God blessed

Re: [Qemu-devel] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Dr. David Alan Gilbert]

* Chen Gang (gang.chen.5i5j@gmail.com) wrote:
> Call g_free() after cache_fini() in migration_end(), but do not call
> g_free() after call cache_fini() in xbzrle_cache_resize() which will
> cause memory leak.
> 
> cache_init() and cache_fini() are pair, so need let cache_fini() call
> g_free(cache) to match cache_init(), then fix current issue too.
> 
> 
> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com>

Yes, good catch.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  arch_init.c  | 1 -
>  page_cache.c | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch_init.c b/arch_init.c
> index 9f1a174..23044c1 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -739,7 +739,6 @@ static void migration_end(void)
>      XBZRLE_cache_lock();
>      if (XBZRLE.cache) {
>          cache_fini(XBZRLE.cache);
> -        g_free(XBZRLE.cache);
>          g_free(XBZRLE.encoded_buf);
>          g_free(XBZRLE.current_buf);
>          XBZRLE.cache = NULL;
> diff --git a/page_cache.c b/page_cache.c
> index b033681..89bb1ec 100644
> --- a/page_cache.c
> +++ b/page_cache.c
> @@ -109,6 +109,7 @@ void cache_fini(PageCache *cache)
>  
>      g_free(cache->page_cache);
>      cache->page_cache = NULL;
> +    g_free(cache);
>  }
>  
>  static size_t cache_get_cache_pos(const PageCache *cache,
> -- 
> 1.7.11.7
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [Qemu-trivial] [PATCH-trivial] arch_init.c: Free 'cache' in cache_fini() to avoid memory leak

[Michael Tokarev]

02.06.2014 16:16, Chen Gang wrote:
> Call g_free() after cache_fini() in migration_end(), but do not call
> g_free() after call cache_fini() in xbzrle_cache_resize() which will
> cause memory leak.
> 
> cache_init() and cache_fini() are pair, so need let cache_fini() call
> g_free(cache) to match cache_init(), then fix current issue too.

Applied to -trivial, with the following commit message:

migration: Plug memory leak in migrate-set-cache-size command

We call g_free() after cache_fini() in migration_end(), but we don't
call it after cache_fini() in xbzrle_cache_resize(), leaking the
memory.

cache_init() and cache_fini() are a pair.  Since cache_init()
allocates the cache, let cache_fini() free it.  This plugs the leak.

Thanks,

/mjt