When using symbol filter in hists browser, some unusal operations
causes segfault. This patchset avoid those crashes.
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Namhyung Kim <[email protected]>
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
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 <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
---
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
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 <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Namhyung Kim <[email protected]>
---
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
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 <[email protected]>
> Acked-by: Namhyung Kim <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> ---
> 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
>
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 <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
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
>
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 <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
Acked-by: Namhyung Kim <[email protected]>
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
>