Re: [Qemu-block] [PATCH v1 1/1] qemu/migration: fix the double free problem on from_src_file

[Peter Xu]

On Tue, Jun 06, 2017 at 11:38:05AM +0800, QingFeng Hao wrote:
> 
> 
> 在 2017/6/6 11:03, Peter Xu 写道:
> >On Mon, Jun 05, 2017 at 12:48:51PM +0200, QingFeng Hao wrote:
> >>In load_vmstate, mis->from_src_file is freed twice, the first free is by
> >>qemu_fclose, the second is by migration_incoming_state_destroy and
> >>it causes Illegal instruction exception. The fix is just to remove the
> >>first free.
> >>
> >>This problem is found by qemu-iotests case 068 since commit
> >>"660819b migration: shut src return path unconditionally". The error is:
> >>068 1s ... - output mismatch (see 068.out.bad)
> >>     --- tests/qemu-iotests/068.out 2017-05-06 01:00:26.417270437 +0200
> >>     +++ 068.out.bad        2017-06-03 13:59:55.360274640 +0200
> >>     @@ -6,6 +6,8 @@
> >>      QEMU X.Y.Z monitor - type 'help' for more information
> >>      (qemu) savevm 0
> >>      (qemu) quit
> >>     +./common.config: line 107: 242472 Illegal instruction     (core 
> >> dumped) ( if [ -n "${QEMU_NEED_PID}" ]; then
> >>     +    echo $BASHPID > "${QEMU_TEST_DIR}/qemu-${_QEMU_HANDLE}.pid";
> >>     +fi; exec "$QEMU_PROG" $QEMU_OPTIONS "$@" )
> >>      QEMU X.Y.Z monitor - type 'help' for more information
> >>     -(qemu) quit
> >>     -*** done
> >>     +(qemu) *** done
> >>
> >>Signed-off-by: QingFeng Hao <address@hidden>
> >>---
> >>  migration/savevm.c | 1 -
> >>  1 file changed, 1 deletion(-)
> >>
> >>diff --git a/migration/savevm.c b/migration/savevm.c
> >>index 9c320f59d0..853e14e34e 100644
> >>--- a/migration/savevm.c
> >>+++ b/migration/savevm.c
> >>@@ -2290,7 +2290,6 @@ int load_snapshot(const char *name, Error **errp)
> >>      aio_context_acquire(aio_context);
> >>      ret = qemu_loadvm_state(f);
> >>-    qemu_fclose(f);
> >>      aio_context_release(aio_context);
> >>      migration_incoming_state_destroy();
> >Thanks QingFeng for the fix!
> >
> >Reviewed-by: Peter Xu <address@hidden>
> >
> >Though here instead of removing the fclose(), not sure whether this is
> >nicer:
> >
> >diff --git a/migration/savevm.c b/migration/savevm.c
> >index 9c320f5..1feb519 100644
> >--- a/migration/savevm.c
> >+++ b/migration/savevm.c
> >@@ -2233,7 +2233,6 @@ int load_snapshot(const char *name, Error **errp)
> >      QEMUFile *f;
> >      int ret;
> >      AioContext *aio_context;
> >-    MigrationIncomingState *mis = migration_incoming_get_current();
> >
> >      if (!bdrv_all_can_snapshot(&bs)) {
> >          error_setg(errp,
> >@@ -2286,7 +2285,6 @@ int load_snapshot(const char *name, Error **errp)
> >      }
> >
> >      qemu_system_reset(SHUTDOWN_CAUSE_NONE);
> >-    mis->from_src_file = f;
> >
> >      aio_context_acquire(aio_context);
> >      ret = qemu_loadvm_state(f);
> Thanks Peter. I have a doubt on this change because I see there are several
> places
> referencing from_src_file e.g. loadvm_postcopy_ram_handle_discard, and it
> seems to
> get byte from the file and it's called by qemu_loadvm_state.
> Anyway, you remind me for the description that is "In load_snapshot" rather
> than
> "In load_vmstate". thanks

Oh I didn't really notice that. :)

It shouldn't affect imho since load snapshot won't trigger postcopy
codes. But sure current fix is good enough for me at least.

Thanks,

-- 
Peter Xu