Messages in this thread | | | Date | Thu, 3 Dec 2015 10:36:33 +0800 | From | "Wangnan (F)" <> | Subject | Re: [PATCH 1/2] perf hists browser: Add NULL pointer check to prevent crash |
| |
On 2015/12/2 23:59, Namhyung Kim wrote: > On Wed, Dec 02, 2015 at 12:51:32PM +0000, Wang Nan wrote: >> Before this patch we can trigger a segfault by following steps: >> >> 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) > I guess your data file doesn't contain callchains. Otherwise 'ENTER' > key will toggle it and not show context menu.
Yes. With callchain information, this step should be replaced by:
Toggle an entry and select a leaf entry which can be annotated with UP and DOWN, don't press 'ENTER'.
I get similar result.
> We now support 'm' key > to show context menu in any case. > > >> 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 <wangnan0@huawei.com> >> Cc: Arnaldo Carvalho de Melo <acme@redhat.com> >> Cc: Namhyung Kim <namhyung@kernel.org> >> --- >> tools/perf/ui/browsers/hists.c | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c >> index 33da341..9458da8 100644 >> --- a/tools/perf/ui/browsers/hists.c >> +++ b/tools/perf/ui/browsers/hists.c >> @@ -1280,8 +1280,10 @@ static void ui_browser__hists_seek(struct ui_browser *browser, >> * Moves not relative to the first visible entry invalidates its >> * row_offset: >> */ >> - h = rb_entry(browser->top, struct hist_entry, rb_node); >> - h->row_offset = 0; >> + if (browser->top) { >> + h = rb_entry(browser->top, struct hist_entry, rb_node); >> + h->row_offset = 0; >> + } > Did you have a problem with this code? It seems that > ui_browser__hists_init_top() ensures browser->top is initialized. >
No. Add it for safty because I see in following code that browser->top can be NULL.
Will be removed in next version.
Thank you.
| |