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

[Peter Maydell]

On 18 January 2014 08:30, Gonglei (Arei) <address@hidden> wrote:
> Ping?
>
>> -----Original Message-----
>> From: Gonglei (Arei)
>> Sent: Friday, January 10, 2014 4:59 PM
>> To: address@hidden
>> Cc: Luonengjun; Huangweidong (Hardware); chenliang (T)
>> Subject: [PATCH] migration: Fix free XBZRLE decoded_buf wrong
>>
>> Hi,
>>
>> When qemu do live migration with xbzrle, qemu malloc decoded_buf
>> at destniation end but free it at source end.It will crash qemu
>> by double free error in some scenarios.
>>
>> Signed-off-by: chenliang <address@hidden>
>> ---
>>  arch_init.c                   |    9 ++++++++-
>>  include/migration/migration.h |    1 +
>>  migration.c                   |    1 +
>>  3 files changed, 10 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch_init.c b/arch_init.c
>> index e0acbc5..0453f84 100644
>> --- a/arch_init.c
>> +++ b/arch_init.c
>> @@ -572,6 +572,14 @@ uint64_t ram_bytes_total(void)
>>      return total;
>>  }
>>
>> +void free_xbzrle_decoded_buf(void)
>> +{
>> +    if (XBZRLE.decoded_buf) {
>> +        g_free(XBZRLE.decoded_buf);
>> +        XBZRLE.decoded_buf = NULL;
>> +    }

g_free(NULL) is guaranteed to do nothing, so you don't
need the if() here.

>> +}
>> +
>>  static void migration_end(void)
>>  {
>>      if (migration_bitmap) {
>> @@ -585,7 +593,6 @@ 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;
>>      }

I wonder if it would be better to split the XBZRLE data structure
into part used by the source side and part used by the destination
side; 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.

thanks
-- PMM