On 30.04.24 11:06, Vladimir Sementsov-Ogievskiy wrote:
On 29.04.24 22:32, Peter Xu wrote:
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?
Hmm. But we want to call migration_incoming_state_destroy() in case when
exit-on-error=false too. And in this case we want to keep the error for further
query-migrate commands.
Actually, I think we shouldn't care about freeing the error for exit() case. I
think, we skip a lot of other cleanups which we normally do in main() in case
of this exit().
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
--
Best regards,
Vladimir
