From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755045AbbDTMOu (ORCPT ); Mon, 20 Apr 2015 08:14:50 -0400 Received: from mail-pa0-f44.google.com ([209.85.220.44]:32906 "EHLO mail-pa0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754052AbbDTMOr (ORCPT ); Mon, 20 Apr 2015 08:14:47 -0400 Date: Mon, 20 Apr 2015 21:13:40 +0900 From: Namhyung Kim To: Jiri Olsa Cc: Arnaldo Carvalho de Melo , Ingo Molnar , Peter Zijlstra , LKML , David Ahern Subject: Re: [PATCH 5/7] perf hists browser: Fix possible memory leak Message-ID: <20150420121340.GC8483@danjae.kornet> References: <1429416255-12070-1-git-send-email-namhyung@kernel.org> <1429416255-12070-6-git-send-email-namhyung@kernel.org> <20150420082415.GB9872@krava.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150420082415.GB9872@krava.redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Apr 20, 2015 at 10:24:15AM +0200, Jiri Olsa wrote: > On Sun, Apr 19, 2015 at 01:04:13PM +0900, Namhyung Kim wrote: > > The options array saves strings for each popup menu item. The number of > > items can be vary according to the currently selected item. So it can > > leak some memory if it's exited from a small item. Fix it by freeing > > all items when loop terminates. > > > > Signed-off-by: Namhyung Kim > > --- > > tools/perf/ui/browsers/hists.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c > > index 8f303eb5044d..cace2df7e561 100644 > > --- a/tools/perf/ui/browsers/hists.c > > +++ b/tools/perf/ui/browsers/hists.c > > @@ -1691,7 +1691,8 @@ skip_annotation: > > "Switch to another data file in PWD") > 0) > > switch_data = nr_options++; > > add_exit_option: > > - options[nr_options++] = (char *)"Exit"; > > + if (asprintf(&options[nr_options], "Exit") > 0) > > + nr_options++; > > this one could cause segfault right? nice catch Yes. But the original code don't cause segfault since it just stops at 'nr_options - 1'. When I changed it to go through the whole array, I found it caused a segfault, so changed it. Thanks, Namhyung > > > retry_popup_menu: > > choice = ui__popup_menu(nr_options, options); > > > > @@ -1812,7 +1813,7 @@ out_free_stack: > > pstack__delete(fstack); > > out: > > hist_browser__delete(browser); > > - free_popup_options(options, nr_options - 1); > > + free_popup_options(options, ARRAY_SIZE(options)); > > return key; > > } > > > > -- > > 2.3.5 > >