On Mon, Apr 29, 2024 at 10:14:24PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> It's bad idea to leave critical section with error object freed, but
> s->error still set, this theoretically may lead to use-after-free
> crash. Let's avoid it.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsement...@yandex-team.ru>
> ---
> migration/migration.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 0d26db47f7..58fd5819bc 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -732,9 +732,19 @@ static void process_incoming_migration_bh(void *opaque)
> migration_incoming_state_destroy();
> }
>
> +static void migrate_error_free(MigrationState *s)
> +{
> + QEMU_LOCK_GUARD(&s->error_mutex);
> + if (s->error) {
> + error_free(s->error);
> + s->error = NULL;
> + }
> +}
> +
> static void coroutine_fn
> process_incoming_migration_co(void *opaque)
> {
> + MigrationState *s = migrate_get_current();
> MigrationIncomingState *mis = migration_incoming_get_current();
> PostcopyState ps;
> int ret;
> @@ -779,11 +789,9 @@ process_incoming_migration_co(void *opaque)
> }
>
> if (ret < 0) {
> - MigrationState *s = migrate_get_current();
> -
> if (migrate_has_error(s)) {
> WITH_QEMU_LOCK_GUARD(&s->error_mutex) {
> - error_report_err(s->error);
> + error_report_err(error_copy(s->error));
This looks like a bugfix, agreed.
> }
> }
> error_report("load of migration failed: %s", strerror(-ret));
> @@ -801,6 +809,7 @@ fail:
> MIGRATION_STATUS_FAILED);
> migration_incoming_state_destroy();
>
> + migrate_error_free(s);
Would migration_incoming_state_destroy() be a better place?
One thing weird is we actually reuses MigrationState*'s error for incoming
too, but so far it looks ok as long as QEMU can't be both src & dst. Then
calling migrate_error_free even in incoming_state_destroy() looks ok.
This patch still looks like containing two changes. Better split them (or
just fix the bug only)?
Thanks,
> exit(EXIT_FAILURE);
> }
>
> @@ -1433,15 +1442,6 @@ bool migrate_has_error(MigrationState *s)
> return qatomic_read(&s->error);
> }
>
> -static void migrate_error_free(MigrationState *s)
> -{
> - QEMU_LOCK_GUARD(&s->error_mutex);
> - if (s->error) {
> - error_free(s->error);
> - s->error = NULL;
> - }
> -}
> -
> static void migrate_fd_error(MigrationState *s, const Error *error)
> {
> assert(s->to_dst_file == NULL);
> --
> 2.34.1
>
--
Peter Xu
