[Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Gonglei (Arei)]

Hi,

This is an update of my patch.
Modifications in v2:
* Removing excess check for g_free
* The structure of XBZRLE is divided into two halves.One is for
* src side, another is for dest side.

Signed-off-by: chenliang <chenliang88@huawei.com>
---
 arch_init.c                   |   23 ++++++++++++++---------
 include/migration/migration.h |    1 +
 migration.c                   |    1 +
 3 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/arch_init.c b/arch_init.c
index 77912e7..9a28a43 100644
--- a/arch_init.c
+++ b/arch_init.c
@@ -164,17 +164,15 @@ static struct {
     uint8_t *encoded_buf;
     /* buffer for storing page content */
     uint8_t *current_buf;
-    /* buffer used for XBZRLE decoding */
-    uint8_t *decoded_buf;
     /* Cache for XBZRLE */
     PageCache *cache;
 } XBZRLE = {
     .encoded_buf = NULL,
     .current_buf = NULL,
-    .decoded_buf = NULL,
     .cache = NULL,
 };
-
+/* buffer used for XBZRLE decoding */
+static uint8_t *xbzrle_decoded_buf = NULL;
 
 int64_t xbzrle_cache_resize(int64_t new_size)
 {
@@ -602,6 +600,12 @@ uint64_t ram_bytes_total(void)
     return total;
 }
 
+void free_xbzrle_decoded_buf(void)
+{
+    g_free(xbzrle_decoded_buf);
+    xbzrle_decoded_buf = NULL;
+}
+
 static void migration_end(void)
 {
     if (migration_bitmap) {
@@ -615,8 +619,9 @@ static void migration_end(void)
         g_free(XBZRLE.cache);
         g_free(XBZRLE.encoded_buf);
         g_free(XBZRLE.current_buf);
-        g_free(XBZRLE.decoded_buf);
         XBZRLE.cache = NULL;
+        XBZRLE.encoded_buf = NULL;
+        XBZRLE.current_buf = NULL;
     }
 }
 
@@ -807,8 +812,8 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
     unsigned int xh_len;
     int xh_flags;
 
-    if (!XBZRLE.decoded_buf) {
-        XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE);
+    if (!xbzrle_decoded_buf) {
+        xbzrle_decoded_buf = g_malloc(TARGET_PAGE_SIZE);
     }
 
     /* extract RLE header */
@@ -825,10 +830,10 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
         return -1;
     }
     /* load data and decode */
-    qemu_get_buffer(f, XBZRLE.decoded_buf, xh_len);
+    qemu_get_buffer(f, xbzrle_decoded_buf, xh_len);
 
     /* decode RLE */
-    ret = xbzrle_decode_buffer(XBZRLE.decoded_buf, xh_len, host,
+    ret = xbzrle_decode_buffer(xbzrle_decoded_buf, xh_len, host,
                                TARGET_PAGE_SIZE);
     if (ret == -1) {
         fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
diff --git a/include/migration/migration.h b/include/migration/migration.h
index bfa3951..3e1e6c7 100644
--- a/include/migration/migration.h
+++ b/include/migration/migration.h
@@ -109,6 +109,7 @@ MigrationState *migrate_get_current(void);
 uint64_t ram_bytes_remaining(void);
 uint64_t ram_bytes_transferred(void);
 uint64_t ram_bytes_total(void);
+void free_xbzrle_decoded_buf(void);
 
 void acct_update_position(QEMUFile *f, size_t size, bool zero);
 
diff --git a/migration.c b/migration.c
index 7235c23..3d46804 100644
--- a/migration.c
+++ b/migration.c
@@ -105,6 +105,7 @@ static void process_incoming_migration_co(void *opaque)
 
     ret = qemu_loadvm_state(f);
     qemu_fclose(f);
+    free_xbzrle_decoded_buf();
     if (ret < 0) {
         fprintf(stderr, "load of migration failed\n");
         exit(EXIT_FAILURE);
-- 
1.6.0.2

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Orit Wasserman]

On 01/21/2014 02:11 PM, Gonglei (Arei) wrote:
> Hi,
>
> This is an update of my patch.
> Modifications in v2:
> * Removing excess check for g_free
> * The structure of XBZRLE is divided into two halves.One is for
> * src side, another is for dest side.
>

What is the benefit of splitting the structure?
decode_buf is only allocated (and freed) in the destination any way.

Orit

> Signed-off-by: chenliang <chenliang88@huawei.com>
> ---
>   arch_init.c                   |   23 ++++++++++++++---------
>   include/migration/migration.h |    1 +
>   migration.c                   |    1 +
>   3 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/arch_init.c b/arch_init.c
> index 77912e7..9a28a43 100644
> --- a/arch_init.c
> +++ b/arch_init.c
> @@ -164,17 +164,15 @@ static struct {
>       uint8_t *encoded_buf;
>       /* buffer for storing page content */
>       uint8_t *current_buf;
> -    /* buffer used for XBZRLE decoding */
> -    uint8_t *decoded_buf;
>       /* Cache for XBZRLE */
>       PageCache *cache;
>   } XBZRLE = {
>       .encoded_buf = NULL,
>       .current_buf = NULL,
> -    .decoded_buf = NULL,
>       .cache = NULL,
>   };
> -
> +/* buffer used for XBZRLE decoding */
> +static uint8_t *xbzrle_decoded_buf = NULL;
>
>   int64_t xbzrle_cache_resize(int64_t new_size)
>   {
> @@ -602,6 +600,12 @@ uint64_t ram_bytes_total(void)
>       return total;
>   }
>
> +void free_xbzrle_decoded_buf(void)
> +{
> +    g_free(xbzrle_decoded_buf);
> +    xbzrle_decoded_buf = NULL;
> +}
> +
>   static void migration_end(void)
>   {
>       if (migration_bitmap) {
> @@ -615,8 +619,9 @@ static void migration_end(void)
>           g_free(XBZRLE.cache);
>           g_free(XBZRLE.encoded_buf);
>           g_free(XBZRLE.current_buf);
> -        g_free(XBZRLE.decoded_buf);
>           XBZRLE.cache = NULL;
> +        XBZRLE.encoded_buf = NULL;
> +        XBZRLE.current_buf = NULL;
>       }
>   }
>
> @@ -807,8 +812,8 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
>       unsigned int xh_len;
>       int xh_flags;
>
> -    if (!XBZRLE.decoded_buf) {
> -        XBZRLE.decoded_buf = g_malloc(TARGET_PAGE_SIZE);
> +    if (!xbzrle_decoded_buf) {
> +        xbzrle_decoded_buf = g_malloc(TARGET_PAGE_SIZE);
>       }
>
>       /* extract RLE header */
> @@ -825,10 +830,10 @@ static int load_xbzrle(QEMUFile *f, ram_addr_t addr, void *host)
>           return -1;
>       }
>       /* load data and decode */
> -    qemu_get_buffer(f, XBZRLE.decoded_buf, xh_len);
> +    qemu_get_buffer(f, xbzrle_decoded_buf, xh_len);
>
>       /* decode RLE */
> -    ret = xbzrle_decode_buffer(XBZRLE.decoded_buf, xh_len, host,
> +    ret = xbzrle_decode_buffer(xbzrle_decoded_buf, xh_len, host,
>                                  TARGET_PAGE_SIZE);
>       if (ret == -1) {
>           fprintf(stderr, "Failed to load XBZRLE page - decode error!\n");
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index bfa3951..3e1e6c7 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -109,6 +109,7 @@ MigrationState *migrate_get_current(void);
>   uint64_t ram_bytes_remaining(void);
>   uint64_t ram_bytes_transferred(void);
>   uint64_t ram_bytes_total(void);
> +void free_xbzrle_decoded_buf(void);
>
>   void acct_update_position(QEMUFile *f, size_t size, bool zero);
>
> diff --git a/migration.c b/migration.c
> index 7235c23..3d46804 100644
> --- a/migration.c
> +++ b/migration.c
> @@ -105,6 +105,7 @@ static void process_incoming_migration_co(void *opaque)
>
>       ret = qemu_loadvm_state(f);
>       qemu_fclose(f);
> +    free_xbzrle_decoded_buf();
>       if (ret < 0) {
>           fprintf(stderr, "load of migration failed\n");
>           exit(EXIT_FAILURE);
>

Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Gonglei (Arei)]

> -----Original Message-----
> From: Orit Wasserman [mailto:owasserm@redhat.com]
> Sent: Tuesday, January 21, 2014 8:24 PM
> To: Gonglei (Arei); qemu-devel@nongnu.org; qemu-stable@nongnu.org; Peter
> Maydell; anthony@codemonkey.ws; pbonzini@redhat.com
> Cc: chenliang (T); Luonengjun; Huangweidong (Hardware)
> Subject: Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf
> wrong
> 
> On 01/21/2014 02:11 PM, Gonglei (Arei) wrote:
> > Hi,
> >
> > This is an update of my patch.
> > Modifications in v2:
> > * Removing excess check for g_free
> > * The structure of XBZRLE is divided into two halves.One is for
> > * src side, another is for dest side.
> >
> 
> What is the benefit of splitting the structure?
> decode_buf is only allocated (and freed) in the destination any way.

Yeah, you are right. Splitting the structure is not necessary. 
The key to do that is just for clear logic. As Peter said: 
the current arrangement looks extremely prone to bugs like 
this one where somebody forgets that some of the fields are 
not relevant to whichever of src/dst the code path they're 
writing is used on.

Best regards,
-Gonglei

Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Peter Maydell]

On 21 January 2014 12:24, Orit Wasserman <owasserm@redhat.com> wrote:
> On 01/21/2014 02:11 PM, Gonglei (Arei) wrote:
>>
>> Hi,
>>
>> This is an update of my patch.
>> Modifications in v2:
>> * Removing excess check for g_free
>> * The structure of XBZRLE is divided into two halves.One is for
>> * src side, another is for dest side.
>>
>
> What is the benefit of splitting the structure?
> decode_buf is only allocated (and freed) in the destination any way.

It makes it clearer that it's not all valid and in
use. If you see a struct XBZRLE then the natural
assumption is that when you're done using it you
need to clean up every field in it, which is exactly
the bug that happened here and which this patch fixes.
If the "stuff used by sender" and "stuff used by
receiver" is split, then it's clear that in the sending
code path you need to set up and tear down exactly the
"sending stuff", and in the receiving code path only
the "receiving stuff".

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Eric Blake]

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

On 01/21/2014 05:11 AM, Gonglei (Arei) wrote:
> Hi,

This greeting...

> 
> This is an update of my patch.
> Modifications in v2:

...and this mention of what you changed in v2,...

> * Removing excess check for g_free
> * The structure of XBZRLE is divided into two halves.One is for
> * src side, another is for dest side.
> 
> Signed-off-by: chenliang <chenliang88@huawei.com>
> ---

...belong here, after the --- marker.  Basically, patch emails should
have two portions: the long-lived portion before the --- that will be
stashed into git and which will always be relevant to someone reading
the patch a year from now, and the transient portion after the --- that
is helpful to reviewers that have context about why you are on v2, but
which makes no difference in git (a year from now, we won't care if it
took 1 version or 20 versions to get to the final patch, because git
will only have the final version).

If you need help writing a good commit message, browsing 'git log' will
give you some ideas.  See also http://wiki.qemu.org/Contribute/SubmitAPatch

-- 
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: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Orit Wasserman]

On 01/21/2014 02:58 PM, Gonglei (Arei) wrote:
>
>> -----Original Message-----
>> From: Orit Wasserman [mailto:owasserm@redhat.com]
>> Sent: Tuesday, January 21, 2014 8:24 PM
>> To: Gonglei (Arei); qemu-devel@nongnu.org; qemu-stable@nongnu.org; Peter
>> Maydell; anthony@codemonkey.ws; pbonzini@redhat.com
>> Cc: chenliang (T); Luonengjun; Huangweidong (Hardware)
>> Subject: Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf
>> wrong
>>
>> On 01/21/2014 02:11 PM, Gonglei (Arei) wrote:
>>> Hi,
>>>
>>> This is an update of my patch.
>>> Modifications in v2:
>>> * Removing excess check for g_free
>>> * The structure of XBZRLE is divided into two halves.One is for
>>> * src side, another is for dest side.
>>>
>>
>> What is the benefit of splitting the structure?
>> decode_buf is only allocated (and freed) in the destination any way.
>
> Yeah, you are right. Splitting the structure is not necessary.
> The key to do that is just for clear logic. As Peter said:
> the current arrangement looks extremely prone to bugs like
> this one where somebody forgets that some of the fields are
> not relevant to whichever of src/dst the code path they're
> writing is used on.
>
> Best regards,
> -Gonglei
>

Sounds reasonable.
Thanks for finding the leak and fixing it.

Orit

Orit

Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf wrong

[Gonglei (Arei)]

> -----Original Message-----
> From: Eric Blake [mailto:eblake@redhat.com]
> Sent: Tuesday, January 21, 2014 9:46 PM
> To: Gonglei (Arei); qemu-devel@nongnu.org; qemu-stable@nongnu.org; Peter
> Maydell; anthony@codemonkey.ws; pbonzini@redhat.com
> Cc: chenliang (T); Luonengjun; Huangweidong (Hardware)
> Subject: Re: [Qemu-devel] [PATCH v2] migration:fix free XBZRLE decoded_buf
> wrong
> 
> On 01/21/2014 05:11 AM, Gonglei (Arei) wrote:
> > Hi,
> 
> This greeting...
> 
> >
> > This is an update of my patch.
> > Modifications in v2:
> 
> ...and this mention of what you changed in v2,...
> 
> > * Removing excess check for g_free
> > * The structure of XBZRLE is divided into two halves.One is for
> > * src side, another is for dest side.
> >
> > Signed-off-by: chenliang <chenliang88@huawei.com>
> > ---
> 
> ...belong here, after the --- marker.  Basically, patch emails should
> have two portions: the long-lived portion before the --- that will be
> stashed into git and which will always be relevant to someone reading
> the patch a year from now, and the transient portion after the --- that
> is helpful to reviewers that have context about why you are on v2, but
> which makes no difference in git (a year from now, we won't care if it
> took 1 version or 20 versions to get to the final patch, because git
> will only have the final version).
> 
> If you need help writing a good commit message, browsing 'git log' will
> give you some ideas.  See also http://wiki.qemu.org/Contribute/SubmitAPatch
> 
> --
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org

Thank you so much, Eric. I will learn the guidelines and then send another patch.

Best regards,
-Gonglei