Hi! 08.04.2019 14:33, Yury Kotov wrote: > It fixes heap-use-after-free which was found by clang's ASAN. > > Control flow of this use-after-free: > main_thread: > * Got SIGTERM and completes main loop > * Calls migration_shutdown > - migrate_fd_cancel (so, migration_thread begins to complete) > - object_unref(OBJECT(current_migration)); > > migration_thread: > * migration_iteration_finish -> schedule cleanup bh > * object_unref(OBJECT(s)); (Now, current_migration is freed) > * exits > > main_thread: > * Calls vm_shutdown -> drain bdrvs -> main loop > -> cleanup_bh -> use after free >
[..] > > +static void migrate_fd_cleanup_schedule(MigrationState *s) > +{ > + /* Ref the state for bh, because it may be called when > + * there're already no other refs */ > + object_ref(OBJECT(s)); > + qemu_bh_schedule(s->cleanup_bh); > +} so you ref on scheduling > + > +static void migrate_fd_cleanup_bh(void *opaque) > +{ > + MigrationState *s = opaque; > + migrate_fd_cleanup(s); > + object_unref(OBJECT(s)); > +} and unref after execution of bh. but migration code has also call to qemu_bh_delete(s->cleanup_bh) from migrate_fd_cleanup(), in migrate_fd_cleanup() is called not only from migrate_fd_cleanup_bh.. I mean, that if we call qemu_bh_delete after scheduling, we will not unref our new reference. Still, seems it's impossible, as all other calls to migrate_fd_cleanup are done before migration thread creation. Don't know, should something done around it or not, but decided to mention it. > + > void migrate_set_error(MigrationState *s, const Error *error) > { > qemu_mutex_lock(&s->error_mutex); > @@ -3144,7 +3157,7 @@ static void migration_iteration_finish(MigrationState > *s) > error_report("%s: Unknown ending state %d", __func__, s->state); > break; > } > - qemu_bh_schedule(s->cleanup_bh); > + migrate_fd_cleanup_schedule(s); > qemu_mutex_unlock_iothread(); > } > > @@ -3279,7 +3292,7 @@ void migrate_fd_connect(MigrationState *s, Error > *error_in) > bool resume = s->state == MIGRATION_STATUS_POSTCOPY_PAUSED; > > s->expected_downtime = s->parameters.downtime_limit; > - s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup, s); > + s->cleanup_bh = qemu_bh_new(migrate_fd_cleanup_bh, s); > if (error_in) { > migrate_fd_error(s, error_in); > migrate_fd_cleanup(s); > -- Best regards, Vladimir