[PATCH] perf script: fix crash when processing recorded stat data

[PATCH] perf script: fix crash when processing recorded stat data

[Tony Jones]

While updating Perf to work with Python3 and Python2 I noticed that the 
stat-cpi script was dumping core.

$ perf  stat -e cycles,instructions record -o /tmp/perf.data /bin/false

 Performance counter stats for '/bin/false':

           802,148      cycles

           604,622      instructions                                                       802,148      cycles
           604,622      instructions

       0.001445842 seconds time elapsed

$ perf script -i /tmp/perf.data -s scripts/python/stat-cpi.py
Segmentation fault (core dumped)
...
...
    rblist=rblist@entry=0xb2a200 <rt_stat>,
    new_entry=new_entry@entry=0x7ffcb755c310) at util/rblist.c:33
    ctx=<optimized out>, type=<optimized out>, create=<optimized out>,
    cpu=<optimized out>, evsel=<optimized out>) at util/stat-shadow.c:118
    ctx=<optimized out>, type=<optimized out>, st=<optimized out>)
    at util/stat-shadow.c:196
    count=count@entry=727442, cpu=cpu@entry=0, st=0xb2a200 <rt_stat>)
    at util/stat-shadow.c:239
    config=config@entry=0xafeb40 <stat_config>,
    counter=counter@entry=0x133c6e0) at util/stat.c:372
...
...

The issue is that since 1fcd03946b52 perf_stat__update_shadow_stats now calls
update_runtime_stat passing rt_stat rather than calling update_stats but
perf_stat__init_shadow_stats has never been called to initialize rt_stat in
the script path processing recorded stat data.

Since I can't see any reason why perf_stat__init_shadow_stats() is presently
initialized like it is in builtin-script.c::perf_sample__fprint_metric()
[4bd1bef8bba2f] I'm proposing it instead be initialized once in __cmd_script

Fixes: 1fcd03946b52 ("perf stat: Update per-thread shadow stats")
Signed-off-by: Tony Jones <tonyj@suse.de>
---
 tools/perf/builtin-script.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index d079f36d342d..9a6dd86e606f 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1681,13 +1681,8 @@ static void perf_sample__fprint_metric(struct perf_script *script,
 		.force_header = false,
 	};
 	struct perf_evsel *ev2;
-	static bool init;
 	u64 val;
 
-	if (!init) {
-		perf_stat__init_shadow_stats();
-		init = true;
-	}
 	if (!evsel->stats)
 		perf_evlist__alloc_stats(script->session->evlist, false);
 	if (evsel_script(evsel->leader)->gnum++ == 0)
@@ -2359,6 +2354,8 @@ static int __cmd_script(struct perf_script *script)
 
 	signal(SIGINT, sig_handler);
 
+	perf_stat__init_shadow_stats();
+
 	/* override event processing functions */
 	if (script->show_task_events) {
 		script->tool.comm = process_comm_event;
-- 
2.18.0

Re: [PATCH] perf script: fix crash when processing recorded stat data

[Jiri Olsa]

On Sun, Jan 20, 2019 at 11:14:14AM -0800, Tony Jones wrote:
> While updating Perf to work with Python3 and Python2 I noticed that the 
> stat-cpi script was dumping core.
> 
> $ perf  stat -e cycles,instructions record -o /tmp/perf.data /bin/false
> 
>  Performance counter stats for '/bin/false':
> 
>            802,148      cycles
> 
>            604,622      instructions                                                       802,148      cycles
>            604,622      instructions
> 
>        0.001445842 seconds time elapsed
> 
> $ perf script -i /tmp/perf.data -s scripts/python/stat-cpi.py
> Segmentation fault (core dumped)
> ...
> ...
>     rblist=rblist@entry=0xb2a200 <rt_stat>,
>     new_entry=new_entry@entry=0x7ffcb755c310) at util/rblist.c:33
>     ctx=<optimized out>, type=<optimized out>, create=<optimized out>,
>     cpu=<optimized out>, evsel=<optimized out>) at util/stat-shadow.c:118
>     ctx=<optimized out>, type=<optimized out>, st=<optimized out>)
>     at util/stat-shadow.c:196
>     count=count@entry=727442, cpu=cpu@entry=0, st=0xb2a200 <rt_stat>)
>     at util/stat-shadow.c:239
>     config=config@entry=0xafeb40 <stat_config>,
>     counter=counter@entry=0x133c6e0) at util/stat.c:372
> ...
> ...
> 
> The issue is that since 1fcd03946b52 perf_stat__update_shadow_stats now calls
> update_runtime_stat passing rt_stat rather than calling update_stats but
> perf_stat__init_shadow_stats has never been called to initialize rt_stat in
> the script path processing recorded stat data.
> 
> Since I can't see any reason why perf_stat__init_shadow_stats() is presently
> initialized like it is in builtin-script.c::perf_sample__fprint_metric()
> [4bd1bef8bba2f] I'm proposing it instead be initialized once in __cmd_script
> 
> Fixes: 1fcd03946b52 ("perf stat: Update per-thread shadow stats")
> Signed-off-by: Tony Jones <tonyj@suse.de>
> ---
>  tools/perf/builtin-script.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index d079f36d342d..9a6dd86e606f 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -1681,13 +1681,8 @@ static void perf_sample__fprint_metric(struct perf_script *script,
>  		.force_header = false,
>  	};
>  	struct perf_evsel *ev2;
> -	static bool init;
>  	u64 val;
>  
> -	if (!init) {
> -		perf_stat__init_shadow_stats();
> -		init = true;
> -	}

well, there's no need to use shadow stats until stat
data is processed.. but it's actually just a static
initialization, so there's no need for late init

Reviewed-by: Jiri Olsa <jolsa@kernel.org>

thanks,
jirka

>  	if (!evsel->stats)
>  		perf_evlist__alloc_stats(script->session->evlist, false);
>  	if (evsel_script(evsel->leader)->gnum++ == 0)
> @@ -2359,6 +2354,8 @@ static int __cmd_script(struct perf_script *script)
>  
>  	signal(SIGINT, sig_handler);
>  
> +	perf_stat__init_shadow_stats();
> +
>  	/* override event processing functions */
>  	if (script->show_task_events) {
>  		script->tool.comm = process_comm_event;
> -- 
> 2.18.0
> 

Re: [PATCH] perf script: fix crash when processing recorded stat data

[Ravi Bangoria]

On 1/21/19 12:44 AM, Tony Jones wrote:
> While updating Perf to work with Python3 and Python2 I noticed that the 
> stat-cpi script was dumping core.

[...]
> Fixes: 1fcd03946b52 ("perf stat: Update per-thread shadow stats")
> Signed-off-by: Tony Jones <tonyj@suse.de>

Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>

Re: [PATCH] perf script: fix crash when processing recorded stat data

[Arnaldo Carvalho de Melo]

Em Mon, Jan 21, 2019 at 11:16:26AM +0530, Ravi Bangoria escreveu:
> 
> 
> On 1/21/19 12:44 AM, Tony Jones wrote:
> > While updating Perf to work with Python3 and Python2 I noticed that the 
> > stat-cpi script was dumping core.
> 
> [...]
> > Fixes: 1fcd03946b52 ("perf stat: Update per-thread shadow stats")
> > Signed-off-by: Tony Jones <tonyj@suse.de>
> 
> Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>

Thanks, applied. Added your tby and Jiri's rby.

- Arnaldo

[tip:perf/urgent] perf script: Fix crash when processing recorded stat data

[tip-bot for Tony Jones]

Commit-ID:  8bf8c6da53c2265aea365a1de6038f118f522113
Gitweb:     https://git.kernel.org/tip/8bf8c6da53c2265aea365a1de6038f118f522113
Author:     Tony Jones <tonyj@suse.de>
AuthorDate: Sun, 20 Jan 2019 11:14:14 -0800
Committer:  Arnaldo Carvalho de Melo <acme@redhat.com>
CommitDate: Mon, 21 Jan 2019 11:29:07 -0300

perf script: Fix crash when processing recorded stat data

While updating perf to work with Python3 and Python2 I noticed that the
stat-cpi script was dumping core.

$ perf  stat -e cycles,instructions record -o /tmp/perf.data /bin/false

 Performance counter stats for '/bin/false':

           802,148      cycles

           604,622      instructions                                                       802,148      cycles
           604,622      instructions

       0.001445842 seconds time elapsed

$ perf script -i /tmp/perf.data -s scripts/python/stat-cpi.py
Segmentation fault (core dumped)
...
...
    rblist=rblist@entry=0xb2a200 <rt_stat>,
    new_entry=new_entry@entry=0x7ffcb755c310) at util/rblist.c:33
    ctx=<optimized out>, type=<optimized out>, create=<optimized out>,
    cpu=<optimized out>, evsel=<optimized out>) at util/stat-shadow.c:118
    ctx=<optimized out>, type=<optimized out>, st=<optimized out>)
    at util/stat-shadow.c:196
    count=count@entry=727442, cpu=cpu@entry=0, st=0xb2a200 <rt_stat>)
    at util/stat-shadow.c:239
    config=config@entry=0xafeb40 <stat_config>,
    counter=counter@entry=0x133c6e0) at util/stat.c:372
...
...

The issue is that since 1fcd03946b52 perf_stat__update_shadow_stats now calls
update_runtime_stat passing rt_stat rather than calling update_stats but
perf_stat__init_shadow_stats has never been called to initialize rt_stat in
the script path processing recorded stat data.

Since I can't see any reason why perf_stat__init_shadow_stats() is presently
initialized like it is in builtin-script.c::perf_sample__fprint_metric()
[4bd1bef8bba2f] I'm proposing it instead be initialized once in __cmd_script

Committer testing:

After applying the patch:

  # perf script -i /tmp/perf.data -s tools/perf/scripts/python/stat-cpi.py
       0.001970: cpu -1, thread -1 -> cpi 1.709079 (1075684/629394)
  #

No segfault.

Signed-off-by: Tony Jones <tonyj@suse.de>
Reviewed-by: Jiri Olsa <jolsa@kernel.org>
Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Tested-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Cc: Andi Kleen <ak@linux.intel.com>
Cc: Jin Yao <yao.jin@linux.intel.com>
Fixes: 1fcd03946b52 ("perf stat: Update per-thread shadow stats")
Link: http://lkml.kernel.org/r/20190120191414.12925-1-tonyj@suse.de
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-script.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 357906ed1898..ac221f137ed2 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -1681,13 +1681,8 @@ static void perf_sample__fprint_metric(struct perf_script *script,
 		.force_header = false,
 	};
 	struct perf_evsel *ev2;
-	static bool init;
 	u64 val;
 
-	if (!init) {
-		perf_stat__init_shadow_stats();
-		init = true;
-	}
 	if (!evsel->stats)
 		perf_evlist__alloc_stats(script->session->evlist, false);
 	if (evsel_script(evsel->leader)->gnum++ == 0)
@@ -2359,6 +2354,8 @@ static int __cmd_script(struct perf_script *script)
 
 	signal(SIGINT, sig_handler);
 
+	perf_stat__init_shadow_stats();
+
 	/* override event processing functions */
 	if (script->show_task_events) {
 		script->tool.comm = process_comm_event;