Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755237AbbLDM53 (ORCPT ); Fri, 4 Dec 2015 07:57:29 -0500 Received: from mail-pf0-f182.google.com ([209.85.192.182]:36050 "EHLO mail-pf0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752073AbbLDM52 (ORCPT ); Fri, 4 Dec 2015 07:57:28 -0500 X-Greylist: delayed 635 seconds by postgrey-1.27 at vger.kernel.org; Fri, 04 Dec 2015 07:57:28 EST Date: Fri, 4 Dec 2015 21:56:53 +0900 From: Namhyung Kim To: Wang Nan Cc: acme@redhat.com, acme@kernel.org, linux-kernel@vger.kernel.org, pi3orama@163.com, lizefan@huawei.com Subject: Re: [PATCH v2 2/3] perf hists browser: Add NULL pointer check to prevent crash Message-ID: <20151204125653.GC22102@danjae.kornet> References: <1449112095-238769-1-git-send-email-wangnan0@huawei.com> <1449112095-238769-3-git-send-email-wangnan0@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1449112095-238769-3-git-send-email-wangnan0@huawei.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2536 Lines: 88 On Thu, Dec 03, 2015 at 03:08:14AM +0000, Wang Nan wrote: > Before this patch we can trigger a segfault by following steps: > > Step 0: Use 'perf record' to generate a perf.data without callchain > > Step 1: perf report > > Step 2: Use UP/DOWN to select an entry, don't press 'ENTER' > > Step 3: Use '/' to filter symbols, use a filter which returns > empty result > > Step 4: Press 'ENTER' (notice here that the old selection is still > there. This is another problem) > > Step 5: Press 'ENTER' to annotate that symbol > > Step 6: Press 'LEFT' to go out. > > Result: segfault: > > perf: Segmentation fault > -------- backtrace -------- > /home/wangnan/perf[0x53e568] > /lib64/libc.so.6(+0x3545f)[0x7fba75d3245f] > /home/wangnan/perf[0x537516] > /home/wangnan/perf[0x533fef] > /home/wangnan/perf[0x53b347] > /home/wangnan/perf(perf_evlist__tui_browse_hists+0x96)[0x53d206] > /home/wangnan/perf(cmd_report+0x1b9f)[0x442c7f] > /home/wangnan/perf[0x47efa2] > /home/wangnan/perf(main+0x5f5)[0x432fa5] > /lib64/libc.so.6(__libc_start_main+0xf4)[0x7fba75d1ebd4] > /home/wangnan/perf[0x4330d4] > > This is because in this case 'nd' could be NULL in > ui_browser__hists_seek(), but that function never check it. > > This patch adds checker for potential NULL pointer in that function. > After this patch the above steps won't segfault again. > > Signed-off-by: Wang Nan > Cc: Arnaldo Carvalho de Melo Acked-by: Namhyung Kim A nitpick below.. > --- > tools/perf/ui/browsers/hists.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c > index 601a585..7447515 100644 > --- a/tools/perf/ui/browsers/hists.c > +++ b/tools/perf/ui/browsers/hists.c > @@ -1297,6 +1297,9 @@ static void ui_browser__hists_seek(struct ui_browser *browser, > * and stop when we printed enough lines to fill the screen. > */ > do_offset: > + > + if (!nd) > + return; Just a style comment, not serious. I prefer the blank line is under the if statement like below.. do_offset: + if (!nd) + return; + Thanks, Namhyung > if (offset > 0) { > do { > h = rb_entry(nd, struct hist_entry, rb_node); > -- > 1.8.3.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/