> -----Original Message-----
> From: Peter Maydell [mailto:peter.mayd...@linaro.org]
> Sent: Saturday, January 18, 2014 11:00 PM
> To: Gonglei (Arei)
> Cc: quint...@redhat.com; anth...@codemonkey.ws; pbonz...@redhat.com;
> chenliang (T); qemu-sta...@nongnu.org; Luonengjun;
> qemu-devel@nongnu.org; Huangweidong (Hardware)
> Subject: Re: [Qemu-devel] [PATCH] migration: Fix free XBZRLE decoded_buf
> wrong
>
> On 18 January 2014 08:30, Gonglei (Arei) <arei.gong...@huawei.com> wrote:
> > Ping?
> >
> >> -----Original Message-----
> >> From: Gonglei (Arei)
> >> Sent: Friday, January 10, 2014 4:59 PM
> >> To: qemu-devel@nongnu.org
> >> 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 <chenlian...@huawei.com>
> >> ---
> >> 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
