[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v5 17/21] migration: Export tls-[creds|hostname|authz] params
From: |
Daniel P . Berrangé |
Subject: |
Re: [PATCH v5 17/21] migration: Export tls-[creds|hostname|authz] params to cmdline too |
Date: |
Thu, 12 May 2022 20:05:45 +0100 |
User-agent: |
Mutt/2.2.1 (2022-02-19) |
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
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|