Re: [Qemu-devel] [PATCH v2 2/3] migration: fix memory leak when updating tls-creds and tls-hostname

[Dr. David Alan Gilbert]

* Peter Xu (address@hidden) wrote:
> On Fri, Jan 11, 2019 at 02:37:31PM +0800, address@hidden wrote:
> > From: Xiao Guangrong <address@hidden>
> > 
> > If we update parameter, tls-creds and tls-hostname, these string
> > values are duplicated to local variables in migrate_params_test_apply()
> > by using g_strdup(), however these new allocated memory are missed to
> > be freed
> > 
> > Actually, they are not used to check anything, we can directly skip
> > them
> > 
> > Signed-off-by: Xiao Guangrong <address@hidden>
> > ---
> >  migration/migration.c | 10 ----------
> >  1 file changed, 10 deletions(-)
> > 
> > diff --git a/migration/migration.c b/migration/migration.c
> > index a82d594f29..fb39d7bec1 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -1145,16 +1145,6 @@ static void 
> > migrate_params_test_apply(MigrateSetParameters *params,
> >          dest->cpu_throttle_increment = params->cpu_throttle_increment;
> >      }
> >  
> > -    if (params->has_tls_creds) {
> > -        assert(params->tls_creds->type == QTYPE_QSTRING);
> > -        dest->tls_creds = g_strdup(params->tls_creds->u.s);
> > -    }
> > -
> > -    if (params->has_tls_hostname) {
> > -        assert(params->tls_hostname->type == QTYPE_QSTRING);
> > -        dest->tls_hostname = g_strdup(params->tls_hostname->u.s);
> > -    }
> > -
> 
> Hi, Guangrong,
> 
> The memleak seems to be correct here but before that I'm even a bit
> confused on why we need to copy the whole parameter list here instead
> of checking against a MigrateSetParameters* in migrate_params_check().
> Could anyone shed some light?  CC Markus too.

I think the problem is that
migrate_params_check checks a MigrationParameters

while the QMP command gives us a MigrateSetParameters; but we also use
migrate_params_check for the global check you added (8b0b29dc) which is
against migrationParameters; so that's why migrate_params_check takes
a MigrationParameters.

It's horrible we've got stuff duped so much.

However, I don't like this fix because if someone later was to add
a test for tls parameters to migrate_params_check, then they would be
confused why the hostname/creds weren't checked.
So while we have migrate_params_test_apply, it should cover all
parameters.

I think a cleaner check would be to write a MigrateParameters_free
that free'd any strings, and call that in qmp_migrate_set_parameters
on both exit paths.

Dave

> Thanks,
> 
> >      if (params->has_max_bandwidth) {
> >          dest->max_bandwidth = params->max_bandwidth;
> >      }
> > -- 
> > 2.14.5
> > 
> 
> Regards,
> 
> -- 
> Peter Xu
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK