diff mbox series

[v2,2/3] migration: fix memory leak when updating tls-creds and tls-hostname

Message ID 20190111063732.10484-3-xiaoguangrong@tencent.com (mailing list archive)
State New, archived
Headers show
Series optimize waiting for free thread to do compression | expand

Commit Message

Xiao Guangrong Jan. 11, 2019, 6:37 a.m. UTC 
From: Xiao Guangrong <xiaoguangrong@tencent.com>

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 <xiaoguangrong@tencent.com>
---
 migration/migration.c | 10 ----------
 1 file changed, 10 deletions(-)

Comments

Peter Xu Jan. 15, 2019, 7:51 a.m. UTC | #1 
On Fri, Jan 11, 2019 at 02:37:31PM +0800, guangrong.xiao@gmail.com wrote:
> From: Xiao Guangrong <xiaoguangrong@tencent.com>
> 
> 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 <xiaoguangrong@tencent.com>
> ---
>  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.

Thanks,

>      if (params->has_max_bandwidth) {
>          dest->max_bandwidth = params->max_bandwidth;
>      }
> -- 
> 2.14.5
> 

Regards,
Dr. David Alan Gilbert Jan. 15, 2019, 10:24 a.m. UTC | #2 
* Peter Xu (peterx@redhat.com) wrote:
> On Fri, Jan 11, 2019 at 02:37:31PM +0800, guangrong.xiao@gmail.com wrote:
> > From: Xiao Guangrong <xiaoguangrong@tencent.com>
> > 
> > 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 <xiaoguangrong@tencent.com>
> > ---
> >  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 / dgilbert@redhat.com / Manchester, UK
Eric Blake Jan. 15, 2019, 4:03 p.m. UTC | #3 
On 1/15/19 4:24 AM, Dr. David Alan Gilbert wrote:

> 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.

Indeed.

> 
> 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.

We already have it; it's named qapi_free_MigrationParameters(),
generated in qapi-types-migration.h.
Peter Xu Jan. 16, 2019, 5:55 a.m. UTC | #4 
On Tue, Jan 15, 2019 at 10:03:53AM -0600, Eric Blake wrote:
> On 1/15/19 4:24 AM, Dr. David Alan Gilbert wrote:
> 
> > 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.
> 
> Indeed.
> 
> > 
> > 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.
> 
> We already have it; it's named qapi_free_MigrationParameters(),
> generated in qapi-types-migration.h.

Yes this seems better.  Then IIUC patch 3 can be simplified as well
with it.

I'm doing one step back and reading below thread for more context that
I've missed:

  https://lists.gnu.org/archive/html/qemu-devel/2017-07/msg04526.html

Do we have chance/plan to remove these duplication for QEMU 4.0?

Thanks,

> 
> -- 
> Eric Blake, Principal Software Engineer
> Red Hat, Inc.           +1-919-301-3226
> Virtualization:  qemu.org | libvirt.org
> 




Regards,
Xiao Guangrong Feb. 18, 2019, 8:26 a.m. UTC | #5 
On 1/16/19 12:03 AM, Eric Blake wrote:
> On 1/15/19 4:24 AM, Dr. David Alan Gilbert wrote:
> 
>> 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.
> 
> Indeed.
> 
>>
>> 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.
> 
> We already have it; it's named qapi_free_MigrationParameters(),
> generated in qapi-types-migration.h.
> 

Thank you all (and sorry for the delay reply due to Chinese New Year :)),
i will use this interface instead in the next version.
diff mbox series

Patch

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);
-    }
-
     if (params->has_max_bandwidth) {
         dest->max_bandwidth = params->max_bandwidth;
     }