| Message ID | 20220425233847.10393-18-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | migration: Postcopy Preemption | expand |
* Peter Xu (peterx@redhat.com) wrote: > It's useful for specifying tls credentials all in the cmdline (along with > the -object tls-creds-*), especially for debugging purpose. > > The trick here is we must remember to not free these fields again in the > finalize() function of migration object, otherwise it'll cause double-free. > > The thing is when destroying an object, we'll first destroy the properties > that bound to the object, then the object itself. To be explicit, when > destroy the object in object_finalize() we have such sequence of > operations: > > object_property_del_all(obj); > object_deinit(obj, ti); > > So after this change the two fields are properly released already even > before reaching the finalize() function but in object_property_del_all(), > hence we don't need to free them anymore in finalize() or it's double-free. > > This also fixes a trivial memory leak for tls-authz as we forgot to free it > before this patch. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > migration/migration.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/migration/migration.c b/migration/migration.c > index 71a50b5c37..b0f2de1e09 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -4339,6 +4339,9 @@ static Property migration_properties[] = { > DEFAULT_MIGRATE_ANNOUNCE_STEP), > DEFINE_PROP_BOOL("x-postcopy-preempt-break-huge", MigrationState, > postcopy_preempt_break_huge, true), > + DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds), > + DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname), > + DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz), > > /* Migration capabilities */ > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > @@ -4372,12 +4375,9 @@ static void migration_class_init(ObjectClass *klass, void *data) > static void migration_instance_finalize(Object *obj) > { > MigrationState *ms = MIGRATION_OBJ(obj); > - MigrationParameters *params = &ms->parameters; > > qemu_mutex_destroy(&ms->error_mutex); > qemu_mutex_destroy(&ms->qemu_file_lock); > - g_free(params->tls_hostname); > - g_free(params->tls_creds); So hmm, why is tls-authz special here? Dave > qemu_sem_destroy(&ms->wait_unplug_sem); > qemu_sem_destroy(&ms->rate_limit_sem); > qemu_sem_destroy(&ms->pause_sem); > -- > 2.32.0 >
On Thu, May 12, 2022 at 07:03:13PM +0100, Dr. David Alan Gilbert wrote: > * Peter Xu (peterx@redhat.com) wrote: > > It's useful for specifying tls credentials all in the cmdline (along with > > the -object tls-creds-*), especially for debugging purpose. > > > > The trick here is we must remember to not free these fields again in the > > finalize() function of migration object, otherwise it'll cause double-free. > > > > The thing is when destroying an object, we'll first destroy the properties > > that bound to the object, then the object itself. To be explicit, when > > destroy the object in object_finalize() we have such sequence of > > operations: > > > > object_property_del_all(obj); > > object_deinit(obj, ti); > > > > So after this change the two fields are properly released already even > > before reaching the finalize() function but in object_property_del_all(), > > hence we don't need to free them anymore in finalize() or it's double-free. > > > > This also fixes a trivial memory leak for tls-authz as we forgot to free it > > before this patch. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > migration/migration.c | 6 +++--- > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/migration/migration.c b/migration/migration.c > > index 71a50b5c37..b0f2de1e09 100644 > > --- a/migration/migration.c > > +++ b/migration/migration.c > > @@ -4339,6 +4339,9 @@ static Property migration_properties[] = { > > DEFAULT_MIGRATE_ANNOUNCE_STEP), > > DEFINE_PROP_BOOL("x-postcopy-preempt-break-huge", MigrationState, > > postcopy_preempt_break_huge, true), > > + DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds), > > + DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname), > > + DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz), > > > > /* Migration capabilities */ > > DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), > > @@ -4372,12 +4375,9 @@ static void migration_class_init(ObjectClass *klass, void *data) > > static void migration_instance_finalize(Object *obj) > > { > > MigrationState *ms = MIGRATION_OBJ(obj); > > - MigrationParameters *params = &ms->parameters; > > > > qemu_mutex_destroy(&ms->error_mutex); > > qemu_mutex_destroy(&ms->qemu_file_lock); > > - g_free(params->tls_hostname); > > - g_free(params->tls_creds); > > So hmm, why is tls-authz special here? Pre-existing memory leak bug IIUC With regards, Daniel
On Thu, May 12, 2022 at 08:05:45PM +0100, Daniel P. Berrangé wrote: > > > @@ -4372,12 +4375,9 @@ static void migration_class_init(ObjectClass *klass, void *data) > > > static void migration_instance_finalize(Object *obj) > > > { > > > MigrationState *ms = MIGRATION_OBJ(obj); > > > - MigrationParameters *params = &ms->parameters; > > > > > > qemu_mutex_destroy(&ms->error_mutex); > > > qemu_mutex_destroy(&ms->qemu_file_lock); > > > - g_free(params->tls_hostname); > > > - g_free(params->tls_creds); > > > > So hmm, why is tls-authz special here? > > Pre-existing memory leak bug IIUC Right, and there's one extra paragraph in commit message explaining it (per Dan's request): This also fixes a trivial memory leak for tls-authz as we forgot to free it before this patch. Thanks,
diff --git a/migration/migration.c b/migration/migration.c index 71a50b5c37..b0f2de1e09 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -4339,6 +4339,9 @@ static Property migration_properties[] = { DEFAULT_MIGRATE_ANNOUNCE_STEP), DEFINE_PROP_BOOL("x-postcopy-preempt-break-huge", MigrationState, postcopy_preempt_break_huge, true), + DEFINE_PROP_STRING("tls-creds", MigrationState, parameters.tls_creds), + DEFINE_PROP_STRING("tls-hostname", MigrationState, parameters.tls_hostname), + DEFINE_PROP_STRING("tls-authz", MigrationState, parameters.tls_authz), /* Migration capabilities */ DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE), @@ -4372,12 +4375,9 @@ static void migration_class_init(ObjectClass *klass, void *data) static void migration_instance_finalize(Object *obj) { MigrationState *ms = MIGRATION_OBJ(obj); - MigrationParameters *params = &ms->parameters; qemu_mutex_destroy(&ms->error_mutex); qemu_mutex_destroy(&ms->qemu_file_lock); - g_free(params->tls_hostname); - g_free(params->tls_creds); qemu_sem_destroy(&ms->wait_unplug_sem); qemu_sem_destroy(&ms->rate_limit_sem); qemu_sem_destroy(&ms->pause_sem);
It's useful for specifying tls credentials all in the cmdline (along with the -object tls-creds-*), especially for debugging purpose. The trick here is we must remember to not free these fields again in the finalize() function of migration object, otherwise it'll cause double-free. The thing is when destroying an object, we'll first destroy the properties that bound to the object, then the object itself. To be explicit, when destroy the object in object_finalize() we have such sequence of operations: object_property_del_all(obj); object_deinit(obj, ti); So after this change the two fields are properly released already even before reaching the finalize() function but in object_property_del_all(), hence we don't need to free them anymore in finalize() or it's double-free. This also fixes a trivial memory leak for tls-authz as we forgot to free it before this patch. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)