From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Dr. David Alan Gilbert" Subject: Re: [PATCH v2 2/3] migration: fix memory leak when updating tls-creds and tls-hostname Date: Tue, 15 Jan 2019 10:24:04 +0000 Message-ID: <20190115102404.GA2135@work-vm> References: <20190111063732.10484-1-xiaoguangrong@tencent.com> <20190111063732.10484-3-xiaoguangrong@tencent.com> <20190115075154.GI24343@xz-x1> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org, mst@redhat.com, mtosatti@redhat.com, Xiao Guangrong , qemu-devel@nongnu.org, Markus Armbruster , quintela@redhat.com, wei.w.wang@intel.com, cota@braap.org, guangrong.xiao@gmail.com, pbonzini@redhat.com To: Peter Xu Return-path: Content-Disposition: inline In-Reply-To: <20190115075154.GI24343@xz-x1> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+gceq-qemu-devel2=m.gmane.org@nongnu.org Sender: "Qemu-devel" List-Id: kvm.vger.kernel.org * Peter Xu (peterx@redhat.com) wrote: > On Fri, Jan 11, 2019 at 02:37:31PM +0800, guangrong.xiao@gmail.com wrote: > > From: Xiao Guangrong > > > > 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 > > --- > > 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