| Message ID | 20220331150857.74406-16-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | migration: Postcopy Preemption | expand |
On Thu, Mar 31, 2022 at 11:08:53AM -0400, Peter Xu 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. I believe this is also fixing a small memory leak > > 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 899084f993..1dc80be1f4 100644 > --- a/migration/migration.c > +++ b/migration/migration.c > @@ -4349,6 +4349,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), > @@ -4382,12 +4385,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); tls_authz wasn't previously freed here, and now it will be > qemu_sem_destroy(&ms->wait_unplug_sem); > qemu_sem_destroy(&ms->rate_limit_sem); > qemu_sem_destroy(&ms->pause_sem); With regards, Daniel
On Wed, Apr 20, 2022 at 12:13:07PM +0100, Daniel P. Berrangé wrote: > On Thu, Mar 31, 2022 at 11:08:53AM -0400, Peter Xu 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. > > > I believe this is also fixing a small memory leak Yes I think so. I didn't even mention it since it's one global tiny variable and IIUC QEMU does have other similar cases of keeping vars around. As long as it'll not grow dynamically, then doesn't sound like a huge problem. But yeah, doing proper free is still ideal. So I'll add one more sentence to the commit message in next version. Thanks,
diff --git a/migration/migration.c b/migration/migration.c index 899084f993..1dc80be1f4 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -4349,6 +4349,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), @@ -4382,12 +4385,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. Signed-off-by: Peter Xu <peterx@redhat.com> --- migration/migration.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)