From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754514AbdDDPvK (ORCPT ); Tue, 4 Apr 2017 11:51:10 -0400 Received: from mail.kernel.org ([198.145.29.136]:38036 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752202AbdDDPvJ (ORCPT ); Tue, 4 Apr 2017 11:51:09 -0400 Date: Tue, 4 Apr 2017 12:51:03 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Jiri Olsa , changbin.du@intel.com, Peter Zijlstra , Ingo Molnar , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v2] perf: fix double free at function perf_hpp__reset_output_field Message-ID: <20170404155103.GF12903@kernel.org> References: <20170315021631.31980-1-changbin.du@intel.com> <20170327062255.27309-1-changbin.du@intel.com> <20170404151940.GD12903@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.8.0 (2017-02-23) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Wed, Apr 05, 2017 at 12:34:59AM +0900, Namhyung Kim escreveu: > Hi Arnaldo, > > On Wed, Apr 5, 2017 at 12:19 AM, Arnaldo Carvalho de Melo > wrote: > > Em Mon, Mar 27, 2017 at 02:22:55PM +0800, changbin.du@intel.com escreveu: > >> From: Changbin Du > >> > >> Some perf_hpp_fmt both registered at field and sort list. For such > >> instance, we only can free it when removed from the both lists. This > >> function currently only used by self-test code, but still should fix > >> it. > > > > Looks sane, applying, > > > > Jiri, Namhyung, please holler (or ack) if needed, > > Did you actually see the double free problem? AFAICS the old code I assumed that he had seen it, in some self-test code, Changbin, can you please show command output or further describe when this patch would be necessary? - Arnaldo > removed a fmt from both list before free it. In the first loop, fmt that > was linked to both output list and sort list will be remove. And the > second loop frees fmt that was linked only to the sort list (IOW, it > frees fmt that was not freed in the first loop). > > Thanks, > Namhyung > > > > > > - Arnaldo > > > >> Signed-off-by: Changbin Du > >> --- > >> v2: removed redundant Signed-off. > >> > >> --- > >> tools/perf/ui/hist.c | 25 +++++++++++++++---------- > >> 1 file changed, 15 insertions(+), 10 deletions(-) > >> > >> diff --git a/tools/perf/ui/hist.c b/tools/perf/ui/hist.c > >> index 5d632dc..f94b301 100644 > >> --- a/tools/perf/ui/hist.c > >> +++ b/tools/perf/ui/hist.c > >> @@ -609,20 +609,25 @@ static void fmt_free(struct perf_hpp_fmt *fmt) > >> > >> void perf_hpp__reset_output_field(struct perf_hpp_list *list) > >> { > >> - struct perf_hpp_fmt *fmt, *tmp; > >> + struct perf_hpp_fmt *field_fmt, *sort_fmt, *tmp1, *tmp2; > >> > >> /* reset output fields */ > >> - perf_hpp_list__for_each_format_safe(list, fmt, tmp) { > >> - list_del_init(&fmt->list); > >> - list_del_init(&fmt->sort_list); > >> - fmt_free(fmt); > >> + perf_hpp_list__for_each_format_safe(list, field_fmt, tmp1) { > >> + list_del_init(&field_fmt->list); > >> + /* reset sort keys */ > >> + perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp2) { > >> + if (field_fmt == sort_fmt) { > >> + list_del_init(&field_fmt->sort_list); > >> + break; > >> + } > >> + } > >> + fmt_free(field_fmt); > >> } > >> > >> - /* reset sort keys */ > >> - perf_hpp_list__for_each_sort_list_safe(list, fmt, tmp) { > >> - list_del_init(&fmt->list); > >> - list_del_init(&fmt->sort_list); > >> - fmt_free(fmt); > >> + /* reset remaining sort keys */ > >> + perf_hpp_list__for_each_sort_list_safe(list, sort_fmt, tmp1) { > >> + list_del_init(&sort_fmt->sort_list); > >> + fmt_free(sort_fmt); > >> } > >> } > >> > >> -- > >> 2.7.4