From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Xu Subject: Re: [PATCH v2 2/3] migration: fix memory leak when updating tls-creds and tls-hostname Date: Tue, 15 Jan 2019 15:51:54 +0800 Message-ID: <20190115075154.GI24343@xz-x1> References: <20190111063732.10484-1-xiaoguangrong@tencent.com> <20190111063732.10484-3-xiaoguangrong@tencent.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: kvm@vger.kernel.org, mst@redhat.com, Markus Armbruster , mtosatti@redhat.com, Xiao Guangrong , dgilbert@redhat.com, qemu-devel@nongnu.org, quintela@redhat.com, wei.w.wang@intel.com, cota@braap.org, pbonzini@redhat.com To: guangrong.xiao@gmail.com Return-path: Content-Disposition: inline In-Reply-To: <20190111063732.10484-3-xiaoguangrong@tencent.com> 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 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. Thanks, > if (params->has_max_bandwidth) { > dest->max_bandwidth = params->max_bandwidth; > } > -- > 2.14.5 > Regards, -- Peter Xu