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

[Gonglei (Arei)]

> -----Original Message-----
> From: Peter Maydell [mailto:address@hidden
> Sent: Saturday, January 18, 2014 11:00 PM
> To: Gonglei (Arei)
> Cc: address@hidden; address@hidden; address@hidden;
> chenliang (T); address@hidden; Luonengjun;
> address@hidden; Huangweidong (Hardware)
> Subject: Re: [Qemu-devel] [PATCH] migration: Fix free XBZRLE decoded_buf
> wrong
> 
> 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.

Yes, you are correct.

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

will do, thanks for the feedback.

Best regards,
-Gonglei