[PATCH v2 0/3] perf hists browser: Avoid crash in some unusal operations

[PATCH v2 0/3] perf hists browser: Avoid crash in some unusal operations

[Wang Nan]

When using symbol filter in hists browser, some unusal operations
causes segfault. This patchset avoid those crashes.

Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>

Wang Nan (3):
  perf hists browser: Fix segfault if use symbol filter in cmdline
  perf hists browser: Add NULL pointer check to prevent crash
  perf hists browser: Reset selection when refresh

 tools/perf/ui/browsers/hists.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
1.8.3.4

[PATCH v2 1/3] perf hists browser: Fix segfault if use symbol filter in cmdline

[Wang Nan]

If feed perf a symbol filter in cmdline and the result is empty,
pressing 'Enter' in the hist browser causes crash:

 # ./perf report perf.data   <-- Common mistake for beginners

Then press 'Enter':

 perf: Segmentation fault
 -------- backtrace --------
 /home/wangnan/perf[0x53e578]
 /lib64/libc.so.6(+0x3545f)[0x7f76bafe045f]
 /home/wangnan/perf[0x539dd4]
 /home/wangnan/perf(perf_evlist__tui_browse_hists+0x96)[0x53d216]
 /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)[0x7f76bafccbd4]
 /home/wangnan/perf[0x4330d4]

This is because 'perf.data' is interperted as a symbol filter, and the
result is empty, so selection is empty. However,
hist_browser__toggle_fold() forgets to check it.

This patch simply return false when selection is NULL.

Signed-off-by: Wang Nan <wangnan0@huawei.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 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 dcdcbaf..601a585 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -340,6 +340,9 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)
 	struct callchain_list *cl = container_of(ms, struct callchain_list, ms);
 	bool has_children;
 
+	if (!ms)
+		return false;
+
 	if (ms == &he->ms)
 		has_children = hist_entry__toggle_fold(he);
 	else
-- 
1.8.3.4

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

[Wang Nan]

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 <wangnan0@huawei.com>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
---
 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;
 	if (offset > 0) {
 		do {
 			h = rb_entry(nd, struct hist_entry, rb_node);
-- 
1.8.3.4

[PATCH v2 3/3] 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->{he_,}selection in
hist_browser__refresh() and let it choose default selection. In this
case browser->{he_,}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>
---
 tools/perf/ui/browsers/hists.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7447515..d555ba9 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -1192,6 +1192,8 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
 	}
 
 	ui_browser__hists_init_top(browser);
+	hb->he_selection = NULL;
+	hb->selection = NULL;
 
 	for (nd = browser->top; nd; nd = rb_next(nd)) {
 		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
-- 
1.8.3.4

Re: [PATCH v2 1/3] perf hists browser: Fix segfault if use symbol filter in cmdline

[Namhyung Kim]

On Thu, Dec 03, 2015 at 03:08:13AM +0000, Wang Nan wrote:
> If feed perf a symbol filter in cmdline and the result is empty,
> pressing 'Enter' in the hist browser causes crash:
> 
>  # ./perf report perf.data   <-- Common mistake for beginners
> 
> Then press 'Enter':
> 
>  perf: Segmentation fault
>  -------- backtrace --------
>  /home/wangnan/perf[0x53e578]
>  /lib64/libc.so.6(+0x3545f)[0x7f76bafe045f]
>  /home/wangnan/perf[0x539dd4]
>  /home/wangnan/perf(perf_evlist__tui_browse_hists+0x96)[0x53d216]
>  /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)[0x7f76bafccbd4]
>  /home/wangnan/perf[0x4330d4]
> 
> This is because 'perf.data' is interperted as a symbol filter, and the
> result is empty, so selection is empty. However,
> hist_browser__toggle_fold() forgets to check it.
> 
> This patch simply return false when selection is NULL.
> 
> Signed-off-by: Wang Nan <wangnan0@huawei.com>
> Acked-by: Namhyung Kim <namhyung@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
>  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 dcdcbaf..601a585 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -340,6 +340,9 @@ static bool hist_browser__toggle_fold(struct hist_browser *browser)
>  	struct callchain_list *cl = container_of(ms, struct callchain_list, ms);
>  	bool has_children;
>  
> +	if (!ms)
> +		return false;
> +

I'm ok with this change itself.  But as other code checks the
browser->he_selection, it'd be better to check 'he' here for
consistency.

Thanks,
Namhyung


>  	if (ms == &he->ms)
>  		has_children = hist_entry__toggle_fold(he);
>  	else
> -- 
> 1.8.3.4
> 

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

[Namhyung Kim]

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 <wangnan0@huawei.com>
> Cc: Arnaldo Carvalho de Melo <acme@redhat.com>

Acked-by: Namhyung Kim <namhyung@kernel.org>

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
> 

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

[Namhyung Kim]

On Thu, Dec 03, 2015 at 03:08:15AM +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->{he_,}selection in
> hist_browser__refresh() and let it choose default selection. In this
> case browser->{he_,}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>

Acked-by: Namhyung Kim <namhyung@kernel.org>

Thanks,
Namhyung


> ---
>  tools/perf/ui/browsers/hists.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 7447515..d555ba9 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -1192,6 +1192,8 @@ static unsigned int hist_browser__refresh(struct ui_browser *browser)
>  	}
>  
>  	ui_browser__hists_init_top(browser);
> +	hb->he_selection = NULL;
> +	hb->selection = NULL;
>  
>  	for (nd = browser->top; nd; nd = rb_next(nd)) {
>  		struct hist_entry *h = rb_entry(nd, struct hist_entry, rb_node);
> -- 
> 1.8.3.4
>