[PATCH 1/2] perf hists browser: Add NULL pointer check to prevent crash

[PATCH 1/2] perf hists browser: Add NULL pointer check to prevent crash

[Wang Nan]

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)

 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;
+	}
 
 	/*
 	 * Here we have to check if nd is expanded (+), if it is we can't go
@@ -1297,6 +1299,8 @@ 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;
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-- 
1.8.3.4

[PATCH 2/2] perf hists browser: Reset selection when refresh

[Wang Nan]

With 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'

We see that, even if we have filter all symbols (and the main interface
is empty), pressing 'ENTER' still select one symbol. This behavior
surprise user. This patch resets browser->selection in
hist_browser__refresh() and let it choose default selection. In this
case browser->selection keeps NULL so user won't see annotation item
in menu.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---

Note that if this patch is applied before 1/2 then the steps listed in
commit message in 1/2 won't trigger segfault. However I believe patch 1/1
is still required.

---
 tools/perf/ui/browsers/hists.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 9458da8..523a9ef 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1192,6 +1192,7 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
 	}
 
 	ui_browser__hists_init_top(browser);
+	hb->selection = NULL;
 
 	for (nd = browser->top; nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
@@ -2102,7 +2103,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
 
 		if (browser->he_selection != NULL) {
 			thread = hist_browser__selected_thread(browser);
-			map = browser->selection->map;
+			map = browser->selection ? browser->selection->map : NULL;
 			socked_id = browser->he_selection->socket;
 		}
 		switch (key) {
@@ -2321,7 +2322,8 @@ skip_annotation:
 			nr_options += add_script_opt(browser,
 						     &actions[nr_options],
 						     &options[nr_options],
-						     NULL, browser->selection->sym);
+						     NULL, browser->selection ?
+						     		browser->selection->sym : NULL);
 		}
 		nr_options += add_script_opt(browser, &actions[nr_options],
 					     &options[nr_options], NULL, NULL);
-- 
1.8.3.4

Re: [PATCH 2/2] perf hists browser: Reset selection when refresh

[Namhyung Kim]

On Wed, Dec 02, 2015 at 12:51:33PM +0000, Wang Nan wrote:
> With 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'
> 
> We see that, even if we have filter all symbols (and the main interface
> is empty), pressing 'ENTER' still select one symbol. This behavior
> surprise user. This patch resets browser->selection in
> hist_browser__refresh() and let it choose default selection. In this
> case browser->selection keeps NULL so user won't see annotation item
> in menu.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> ---
> 
> Note that if this patch is applied before 1/2 then the steps listed in
> commit message in 1/2 won't trigger segfault. However I believe patch 1/1
> is still required.
> 
> ---
>  tools/perf/ui/browsers/hists.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 9458da8..523a9ef 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -1192,6 +1192,7 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
>  	}
>  
>  	ui_browser__hists_init_top(browser);
> +	hb->selection = NULL;

This code assumes that hb->selection is not NULL if hb->he_selection
is not NULL.  So I think that the right (and simple) fix is to reset
hb->he_selection rather than hb->selection (or both).  It'll make the
change below unnecessary IMHO.

Thanks,
Namhyung


>  
>  	for (nd = browser->top; nd; nd = rb_next(nd)) {
>  		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
> @@ -2102,7 +2103,7 @@ static int perf_evsel__hists_browse(struct perf_evsel *evsel, int nr_events,
>  
>  		if (browser->he_selection != NULL) {
>  			thread = hist_browser__selected_thread(browser);
> -			map = browser->selection->map;
> +			map = browser->selection ? browser->selection->map : NULL;
>  			socked_id = browser->he_selection->socket;
>  		}
>  		switch (key) {
> @@ -2321,7 +2322,8 @@ skip_annotation:
>  			nr_options += add_script_opt(browser,
>  						     &actions[nr_options],
>  						     &options[nr_options],
> -						     NULL, browser->selection->sym);
> +						     NULL, browser->selection ?
> +						     		browser->selection->sym : NULL);
>  		}
>  		nr_options += add_script_opt(browser, &actions[nr_options],
>  					     &options[nr_options], NULL, NULL);
> -- 
> 1.8.3.4
> 

Re: [PATCH 2/2] perf hists browser: Reset selection when refresh

[Wangnan (F)]

On 2015/12/3 0:17, Namhyung Kim wrote:
> On Wed, Dec 02, 2015 at 12:51:33PM +0000, Wang Nan wrote:
>> With 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'
>>
>> We see that, even if we have filter all symbols (and the main interface
>> is empty), pressing 'ENTER' still select one symbol. This behavior
>> surprise user. This patch resets browser->selection in
>> hist_browser__refresh() and let it choose default selection. In this
>> case browser->selection keeps NULL so user won't see annotation item
>> in menu.
>>
>> Signed-off-by: Wang Nan <wangnan0@huawei.com>
>> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> ---
>>
>> Note that if this patch is applied before 1/2 then the steps listed in
>> commit message in 1/2 won't trigger segfault. However I believe patch 1/1
>> is still required.
>>
>> ---
>>   tools/perf/ui/browsers/hists.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
>> index 9458da8..523a9ef 100644
>> --- a/tools/perf/ui/browsers/hists.c
>> +++ b/tools/perf/ui/browsers/hists.c
>> @@ -1192,6 +1192,7 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
>>   	}
>>   
>>   	ui_browser__hists_init_top(browser);
>> +	hb->selection = NULL;
> This code assumes that hb->selection is not NULL if hb->he_selection
> is not NULL.  So I think that the right (and simple) fix is to reset
> hb->he_selection rather than hb->selection (or both).  It'll make the
> change below unnecessary IMHO.

No. Only set hb->he_selection causes crash:

Step 0: user 'perf record ls' create a perf.data without callchain;
Step 1: perf report
Step 2: choose a annotable entry, don't press 'ENTER'
Step 3: use '/' and enter a filter like 'asdfasdf' which ensure no entry
         would be left
Step 3: Press 'ENTER' twice

Crash:
# ./perf report
perf: Segmentation fault
-------- backtrace --------
./perf[0x53e588]
/tmp/oxygen_root/lib64/libc.so.6(+0x3545f)[0x7f2b4d6da45f]
./perf[0x539e03]
./perf(perf_evlist__tui_browse_hists+0x96)[0x53d226]
./perf(cmd_report+0x1b9f)[0x442c7f]
./perf[0x47efc2]
./perf(main+0x5f5)[0x432fa5]
/tmp/oxygen_root/lib64/libc.so.6(__libc_start_main+0xf4)[0x7f2b4d6c6bd4]
./perf[0x4330d4]


GDB result:

Program received signal SIGSEGV, Segmentation fault.
hist_browser__toggle_fold (browser=0x1f71160) at ui/browsers/hists.c:352
(gdb) bt
#0  hist_browser__toggle_fold (browser=0x1f71160) at ui/browsers/hists.c:352
#1  hist_browser__run (help=0x649038 "For a higher level overview, try: 
perf report --sort comm,dso", browser=0x1f71160) at ui/
browsers/hists.c:539
#2  perf_evsel__hists_browse (evsel=0x19ef140, 
nr_events=nr_events@entry=1, helpline=helpline@entry=0x649038 "For a 
higher leve
l overview, try: perf report --sort comm,dso", 
left_exits=left_exits@entry=false, hbt=hbt@entry=0x0, 
min_pcnt=<optimized out>,
env=0x19ed850) at ui/browsers/hists.c:2101
#3  0x000000000053d227 in perf_evlist__tui_browse_hists 
(evlist=0x19ee730, help=help@entry=0x649038 "For a higher level overvie
w, try: perf report --sort comm,dso", hbt=hbt@entry=0x0, 
min_pcnt=<optimized out>, env=<optimized out>) at ui/browsers/hists.c:
2555
#4  0x0000000000442c80 in report__browse_hists (rep=0x7fffffffca20) at 
builtin-report.c:440
#5  __cmd_report (rep=0x7fffffffca20) at builtin-report.c:576
#6  cmd_report (argc=0, argv=0x7fffffffe590, prefix=<optimized out>) at 
builtin-report.c:962
#7  0x000000000047efc3 in run_builtin (p=p@entry=0x8ff9e0 
<commands+192>, argc=argc@entry=1, argv=argv@entry=0x7fffffffe590) at
  perf.c:387
#8  0x0000000000432fa6 in handle_internal_command (argv=0x7fffffffe590, 
argc=1) at perf.c:448
#9  run_argv (argv=0x7fffffffe310, argcp=0x7fffffffe31c) at perf.c:492
#10 main (argc=1, argv=0x7fffffffe590) at perf.c:609

But setting both of them to NULL in hist_browser__refresh() can avoid 
this crash.

So we have two choice:

1. In hist_browser__refresh() let's set both selection and he_selection 
to NULL;

    By this way after step 3 we won't see annotation options. The 
context menu
    (by pressing ENTER or 'm') is:

Run scripts for all samples
Switch to another data file in PWD
Exit


2. In hist_browser__toggle_fold() let's check both he amd ms.

    By this way we still get annotation and map options in context menu:

Annotate __strcoll_l
Browse map details
Run scripts for all samples
Switch to another data file in PWD
Exit

I'm not sure which one is better because I don't really understand this 
part of
code. But for me the second result is surprising because I have filtered all
entries out in my interface, and I believe I should select nothing, so 
pressing
'ENTER' or 'm' I shall not get annotation option because I don't know 
which entry
would be annotated.

Thank you.

Re: [PATCH 2/2] perf hists browser: Reset selection when refresh

[Namhyung Kim]

Hi,

On Thu, Dec 03, 2015 at 11:05:13AM +0800, Wangnan (F) wrote:
> On 2015/12/3 0:17, Namhyung Kim wrote:
> >On Wed, Dec 02, 2015 at 12:51:33PM +0000, Wang Nan wrote:
> >>With 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'
> >>
> >>We see that, even if we have filter all symbols (and the main interface
> >>is empty), pressing 'ENTER' still select one symbol. This behavior
> >>surprise user. This patch resets browser->selection in
> >>hist_browser__refresh() and let it choose default selection. In this
> >>case browser->selection keeps NULL so user won't see annotation item
> >>in menu.
> >>
> >>Signed-off-by: Wang Nan <wangnan0@huawei.com>
> >>Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> >>Cc: Namhyung Kim <namhyung@kernel.org>
> >>---
> >>
> >>Note that if this patch is applied before 1/2 then the steps listed in
> >>commit message in 1/2 won't trigger segfault. However I believe patch 1/1
> >>is still required.
> >>
> >>---
> >>  tools/perf/ui/browsers/hists.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >>diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> >>index 9458da8..523a9ef 100644
> >>--- a/tools/perf/ui/browsers/hists.c
> >>+++ b/tools/perf/ui/browsers/hists.c
> >>@@ -1192,6 +1192,7 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
> >>  	}
> >>  	ui_browser__hists_init_top(browser);
> >>+	hb->selection = NULL;
> >This code assumes that hb->selection is not NULL if hb->he_selection
> >is not NULL.  So I think that the right (and simple) fix is to reset
> >hb->he_selection rather than hb->selection (or both).  It'll make the
> >change below unnecessary IMHO.
> 
> No. Only set hb->he_selection causes crash:
> 
> Step 0: user 'perf record ls' create a perf.data without callchain;
> Step 1: perf report
> Step 2: choose a annotable entry, don't press 'ENTER'
> Step 3: use '/' and enter a filter like 'asdfasdf' which ensure no entry
>         would be left
> Step 3: Press 'ENTER' twice
> 
> Crash:
> # ./perf report
> perf: Segmentation fault
> -------- backtrace --------
> ./perf[0x53e588]
> /tmp/oxygen_root/lib64/libc.so.6(+0x3545f)[0x7f2b4d6da45f]
> ./perf[0x539e03]
> ./perf(perf_evlist__tui_browse_hists+0x96)[0x53d226]
> ./perf(cmd_report+0x1b9f)[0x442c7f]
> ./perf[0x47efc2]
> ./perf(main+0x5f5)[0x432fa5]
> /tmp/oxygen_root/lib64/libc.so.6(__libc_start_main+0xf4)[0x7f2b4d6c6bd4]
> ./perf[0x4330d4]
> 
> 
> GDB result:
> 
> Program received signal SIGSEGV, Segmentation fault.
> hist_browser__toggle_fold (browser=0x1f71160) at ui/browsers/hists.c:352

So this is same crash you already fixed.  Now I think that it'd be
better to check hb->he_selection instead of hb->selection in that
patch for the sake of consistency.


> (gdb) bt
> #0  hist_browser__toggle_fold (browser=0x1f71160) at ui/browsers/hists.c:352
> #1  hist_browser__run (help=0x649038 "For a higher level overview, try: perf
> report --sort comm,dso", browser=0x1f71160) at ui/
> browsers/hists.c:539
> #2  perf_evsel__hists_browse (evsel=0x19ef140, nr_events=nr_events@entry=1,
> helpline=helpline@entry=0x649038 "For a higher leve
> l overview, try: perf report --sort comm,dso",
> left_exits=left_exits@entry=false, hbt=hbt@entry=0x0, min_pcnt=<optimized
> out>,
> env=0x19ed850) at ui/browsers/hists.c:2101
> #3  0x000000000053d227 in perf_evlist__tui_browse_hists (evlist=0x19ee730,
> help=help@entry=0x649038 "For a higher level overvie
> w, try: perf report --sort comm,dso", hbt=hbt@entry=0x0, min_pcnt=<optimized
> out>, env=<optimized out>) at ui/browsers/hists.c:
> 2555
> #4  0x0000000000442c80 in report__browse_hists (rep=0x7fffffffca20) at
> builtin-report.c:440
> #5  __cmd_report (rep=0x7fffffffca20) at builtin-report.c:576
> #6  cmd_report (argc=0, argv=0x7fffffffe590, prefix=<optimized out>) at
> builtin-report.c:962
> #7  0x000000000047efc3 in run_builtin (p=p@entry=0x8ff9e0 <commands+192>,
> argc=argc@entry=1, argv=argv@entry=0x7fffffffe590) at
>  perf.c:387
> #8  0x0000000000432fa6 in handle_internal_command (argv=0x7fffffffe590,
> argc=1) at perf.c:448
> #9  run_argv (argv=0x7fffffffe310, argcp=0x7fffffffe31c) at perf.c:492
> #10 main (argc=1, argv=0x7fffffffe590) at perf.c:609
> 
> But setting both of them to NULL in hist_browser__refresh() can avoid this
> crash.
> 
> So we have two choice:
> 
> 1. In hist_browser__refresh() let's set both selection and he_selection to
> NULL;
> 
>    By this way after step 3 we won't see annotation options. The context
> menu
>    (by pressing ENTER or 'm') is:
> 
> Run scripts for all samples
> Switch to another data file in PWD
> Exit
> 
> 
> 2. In hist_browser__toggle_fold() let's check both he amd ms.
> 
>    By this way we still get annotation and map options in context menu:
> 
> Annotate __strcoll_l
> Browse map details
> Run scripts for all samples
> Switch to another data file in PWD
> Exit
> 
> I'm not sure which one is better because I don't really understand this part
> of
> code. But for me the second result is surprising because I have filtered all
> entries out in my interface, and I believe I should select nothing, so
> pressing
> 'ENTER' or 'm' I shall not get annotation option because I don't know which
> entry
> would be annotated.

Agreed.  I also think that the choice 1 is the right thing.

Thanks,
Namhyung

Re: [PATCH 1/2] perf hists browser: Add NULL pointer check to prevent crash

[Namhyung Kim]

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.  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.



>  
>  	/*
>  	 * Here we have to check if nd is expanded (+), if it is we can't go
> @@ -1297,6 +1299,8 @@ 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;

So I guess checking 'nd' being NULL here is enough.  And please add a
blank line for readability.

Thanks,
Namhyung


>  	if (offset > 0) {
>  		do {
>  			h = rb_entry(nd, struct hist_entry, rb_node);
> -- 
> 1.8.3.4
> 

Re: [PATCH 1/2] perf hists browser: Add NULL pointer check to prevent crash

[Wangnan (F)]

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.