Hi Vladimir!
13.09.2019, 16:43, "Vladimir Sementsov-Ogievskiy" <vsement...@virtuozzo.com>:
> 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.
>
Hmm.. It's very interesting, thanks! I need more time to understand
is there a problem or not, but after quick look I see one possibility
to achieve a leak: qmp_migrate after cleanup bh schedule and before bh call...
>> +
>> 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
Regards,
Yury
