From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753081Ab3GAHzW (ORCPT ); Mon, 1 Jul 2013 03:55:22 -0400 Received: from mga09.intel.com ([134.134.136.24]:17453 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752457Ab3GAHzV (ORCPT ); Mon, 1 Jul 2013 03:55:21 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.87,972,1363158000"; d="scan'208";a="338224193" Message-ID: <51D13754.4080602@intel.com> Date: Mon, 01 Jul 2013 11:01:24 +0300 From: Adrian Hunter Organization: Intel Finland Oy, Registered Address: PL 281, 00181 Helsinki, Business Identity Code: 0357606 - 4, Domiciled in Helsinki User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130311 Thunderbird/17.0.4 MIME-Version: 1.0 To: David Ahern CC: Arnaldo Carvalho de Melo , linux-kernel@vger.kernel.org, Frederic Weisbecker , Jiri Olsa , Mike Galbraith , Namhyung Kim , Paul Mackerras , Peter Zijlstra , Stephane Eranian Subject: Re: [PATCH V3 06/15] perf tools: fix parse_events_terms() freeing local variable on error path References: <1372409006-8431-1-git-send-email-adrian.hunter@intel.com> <1372409006-8431-7-git-send-email-adrian.hunter@intel.com> <51CDC589.8070306@gmail.com> In-Reply-To: <51CDC589.8070306@gmail.com> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 28/06/13 20:19, David Ahern wrote: > On 6/28/13 2:43 AM, Adrian Hunter wrote: >> The list_head is on the stack, so just free the rest of the list. >> >> Signed-off-by: Adrian Hunter >> --- >> tools/perf/util/parse-events.c | 7 ++++++- >> tools/perf/util/parse-events.h | 1 + >> tools/perf/util/pmu.c | 2 +- >> 3 files changed, 8 insertions(+), 2 deletions(-) >> >> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c >> index 995fc25..d9cb055 100644 >> --- a/tools/perf/util/parse-events.c >> +++ b/tools/perf/util/parse-events.c >> @@ -1231,12 +1231,17 @@ int parse_events_term__clone(struct >> parse_events_term **new, >> term->val.str, term->val.num); >> } >> >> -void parse_events__free_terms(struct list_head *terms) >> +void parse_events__free_terms_only(struct list_head *terms) >> { >> struct parse_events_term *term, *h; >> >> list_for_each_entry_safe(term, h, terms, list) >> free(term); >> +} >> + >> +void parse_events__free_terms(struct list_head *terms) >> +{ >> + parse_events__free_terms_only(terms); >> >> free(terms); >> } > > I still don't understand the reasoning for an _only function. There is only > 1 place that mallocs the list_head and that 1 user should free its own > memory. All of the other users pass a stack variable. No. See parse-events.y The list head is defined as a pointer in the YYTYPE stack element: %union { char *str; u64 num; struct list_head *head; struct parse_events_term *term; } It is malloc'ed when terms are created: event_term { struct list_head *head = malloc(sizeof(*head)); struct parse_events_term *term = $1; ABORT_ON(!head); INIT_LIST_HEAD(head); list_add_tail(&term->list, head); $$ = head; }