[PATCH] perf evsel: Fix 2 memory leaks

[PATCH] perf evsel: Fix 2 memory leaks

[Ian Rogers]

If allocated, perf_pkg_mask and metric_events need freeing.

Reviewed-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a2397ca4d57a..654b79c1f4ac 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1266,6 +1266,8 @@ void evsel__exit(struct evsel *evsel)
 	zfree(&evsel->group_name);
 	zfree(&evsel->name);
 	zfree(&evsel->pmu_name);
+	zfree(&evsel->per_pkg_mask);
+	zfree(&evsel->metric_events);
 	perf_evsel__object.fini(evsel);
 }
 
-- 
2.26.2.645.ge9eca65c58-goog

Re: [PATCH] perf evsel: Fix 2 memory leaks

[Arnaldo Carvalho de Melo]

Em Tue, May 12, 2020 at 04:59:18PM -0700, Ian Rogers escreveu:
> If allocated, perf_pkg_mask and metric_events need freeing.

Applied, were those found with some tool? Or just by visual inspection?

Also I noticed that evsel->metric_events is correctly initialized to
NULL in evsel__init(), but evsel->per_pkg_mask isn't, and while
evsel__new() uses zalloc() it is possible, IIRC, for evsels associated
with hists to be part of a larger struct, so I think the safest way is
to initialize evsel->per_pkg_mask to NULL in evsel__init(), will do it
in a follow up patch.

- Arnaldo
 
> Reviewed-by: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a2397ca4d57a..654b79c1f4ac 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -1266,6 +1266,8 @@ void evsel__exit(struct evsel *evsel)
>  	zfree(&evsel->group_name);
>  	zfree(&evsel->name);
>  	zfree(&evsel->pmu_name);
> +	zfree(&evsel->per_pkg_mask);
> +	zfree(&evsel->metric_events);
>  	perf_evsel__object.fini(evsel);
>  }
>  
> -- 
> 2.26.2.645.ge9eca65c58-goog
> 

-- 

- Arnaldo

Re: [PATCH] perf evsel: Fix 2 memory leaks

[Ian Rogers]

On Wed, May 13, 2020 at 6:36 AM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
> Em Tue, May 12, 2020 at 04:59:18PM -0700, Ian Rogers escreveu:
> > If allocated, perf_pkg_mask and metric_events need freeing.
>
> Applied, were those found with some tool? Or just by visual inspection?
>
> Also I noticed that evsel->metric_events is correctly initialized to
> NULL in evsel__init(), but evsel->per_pkg_mask isn't, and while
> evsel__new() uses zalloc() it is possible, IIRC, for evsels associated
> with hists to be part of a larger struct, so I think the safest way is
> to initialize evsel->per_pkg_mask to NULL in evsel__init(), will do it
> in a follow up patch.
>
> - Arnaldo

Thanks for following up on other issues related to this!

On the tool side I was testing metric code with address sanitizer as
described in Build.txt:
https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/tree/tools/perf/Documentation/Build.txt

Here it is for current kernel/git/acme/linux.git perf/core
$ make -C tools/perf O=/tmp/perf DEBUG=1
EXTRA_CFLAGS='-fno-omit-frame-pointer -fsanitize=address' install
$ /tmp/perf/perf stat -a -M TopDownL1 sleep 1
...
==187042==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 332 byte(s) in 19 object(s) allocated from:
    #0 0x7f23a435f0b5 in strdup (/lib/x86_64-linux-gnu/libasan.so.5+0x920b5)
    #1 0x562e03f83321 in str util/expr.l:75
    #2 0x562e03f84048 in expr_lex util/expr.l:110
    #3 0x562e03f8c2ad in expr_parse /tmp/perf/util/expr-bison.c:1341
    #4 0x562e03f8e5fd in __expr__parse util/expr.c:51
    #5 0x562e03f8e74e in expr__parse util/expr.c:61
    #6 0x562e03eeef65 in generic_metric util/stat-shadow.c:781
    #7 0x562e03ef125f in perf_stat__print_shadow_stats util/stat-shadow.c:1057
    #8 0x562e03ef5661 in printout util/stat-display.c:490
    #9 0x562e03ef8248 in print_counter_aggr util/stat-display.c:820
    #10 0x562e03efb31c in perf_evlist__print_counters util/stat-display.c:1233
    #11 0x562e03c36b9e in print_counters tools/perf/builtin-stat.c:777
    #12 0x562e03c3d79c in cmd_stat tools/perf/builtin-stat.c:2151
    #13 0x562e03d79021 in run_builtin tools/perf/perf.c:312
    #14 0x562e03d7956b in handle_internal_command tools/perf/perf.c:364
    #15 0x562e03d79922 in run_argv tools/perf/perf.c:408
    #16 0x562e03d7a0fe in main tools/perf/perf.c:538
    #17 0x7f23a3bccbba in __libc_start_main ../csu/libc-start.c:308

Direct leak of 332 byte(s) in 19 object(s) allocated from:
    #0 0x7f23a435f0b5 in strdup (/lib/x86_64-linux-gnu/libasan.so.5+0x920b5)
    #1 0x562e03f83321 in str util/expr.l:75
    #2 0x562e03f84048 in expr_lex util/expr.l:110
    #3 0x562e03f8c2ad in expr_parse /tmp/perf/util/expr-bison.c:1341
    #4 0x562e03f8e5fd in __expr__parse util/expr.c:51
    #5 0x562e03f8e96d in expr__find_other util/expr.c:85
    #6 0x562e03e1a830 in __metricgroup__add_metric util/metricgroup.c:503
    #7 0x562e03e1af60 in metricgroup__add_metric util/metricgroup.c:552
    #8 0x562e03e1b11e in metricgroup__add_metric_list util/metricgroup.c:588
    #9 0x562e03e1b5c7 in metricgroup__parse_groups util/metricgroup.c:629
    #10 0x562e03c36ed0 in parse_metric_groups tools/perf/builtin-stat.c:843
    #11 0x562e040478c2 in get_value tools/lib/subcmd/parse-options.c:248
    #12 0x562e04048b05 in parse_short_opt tools/lib/subcmd/parse-options.c:348
    #13 0x562e04049ad3 in parse_options_step
tools/lib/subcmd/parse-options.c:536
    #14 0x562e0404a9f6 in parse_options_subcommand
tools/lib/subcmd/parse-options.c:651
    #15 0x562e03c3c73d in cmd_stat tools/perf/builtin-stat.c:1889
    #16 0x562e03d79021 in run_builtin tools/perf/perf.c:312
    #17 0x562e03d7956b in handle_internal_command tools/perf/perf.c:364
    #18 0x562e03d79922 in run_argv tools/perf/perf.c:408
    #19 0x562e03d7a0fe in main tools/perf/perf.c:538
    #20 0x7f23a3bccbba in __libc_start_main ../csu/libc-start.c:308

Direct leak of 152 byte(s) in 1 object(s) allocated from:
    #0 0x7f23a43d4628 in malloc (/lib/x86_64-linux-gnu/libasan.so.5+0x107628)
    #1 0x562e040504fd in cpu_map__trim_new tools/lib/perf/cpumap.c:79
    #2 0x562e04050c97 in perf_cpu_map__read tools/lib/perf/cpumap.c:149
    #3 0x562e04050d7d in cpu_map__read_all_cpu_map tools/lib/perf/cpumap.c:166
    #4 0x562e04050ea6 in perf_cpu_map__new tools/lib/perf/cpumap.c:181
    #5 0x562e03db3063 in perf_evlist__create_maps util/evlist.c:929
    #6 0x562e03c3d138 in cmd_stat tools/perf/builtin-stat.c:2047
    #7 0x562e03d79021 in run_builtin tools/perf/perf.c:312
    #8 0x562e03d7956b in handle_internal_command tools/perf/perf.c:364
    #9 0x562e03d79922 in run_argv tools/perf/perf.c:408
    #10 0x562e03d7a0fe in main tools/perf/perf.c:538
    #11 0x7f23a3bccbba in __libc_start_main ../csu/libc-start.c:308

Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7f23a43d4a2e in __interceptor_realloc
(/lib/x86_64-linux-gnu/libasan.so.5+0x107a2e)
    #1 0x562e04052b3a in perf_thread_map__realloc tools/lib/perf/threadmap.c:23
    #2 0x562e04052cb5 in perf_thread_map__new_dummy
tools/lib/perf/threadmap.c:47
    #3 0x562e03e77a1d in thread_map__new_by_tid_str util/thread_map.c:255
    #4 0x562e03e77e17 in thread_map__new_str util/thread_map.c:304
    #5 0x562e03db2ffe in perf_evlist__create_maps util/evlist.c:920
    #6 0x562e03c3d138 in cmd_stat tools/perf/builtin-stat.c:2047
    #7 0x562e03d79021 in run_builtin tools/perf/perf.c:312
    #8 0x562e03d7956b in handle_internal_command tools/perf/perf.c:364
    #9 0x562e03d79922 in run_argv tools/perf/perf.c:408
    #10 0x562e03d7a0fe in main tools/perf/perf.c:538
    #11 0x7f23a3bccbba in __libc_start_main ../csu/libc-start.c:308

SUMMARY: AddressSanitizer: 848 byte(s) leaked in 40 allocation(s).

The RFC metric group event sharing code fixes the top two, the bottom
two need more investigation due to the use of reference counting. If
each reference count were a leak then it'd be reasonably easy to track
and fix the culprit. There might be something smarter to track down
reference count leaks, I'll try to find out. Setting breakpoints in
gdb I saw about 115 increments and 112 decrements, and pairing those
up was more than a 5 minute job :-)

Thanks,
Ian


> > Reviewed-by: Andi Kleen <ak@linux.intel.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/evsel.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index a2397ca4d57a..654b79c1f4ac 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -1266,6 +1266,8 @@ void evsel__exit(struct evsel *evsel)
> >       zfree(&evsel->group_name);
> >       zfree(&evsel->name);
> >       zfree(&evsel->pmu_name);
> > +     zfree(&evsel->per_pkg_mask);
> > +     zfree(&evsel->metric_events);
> >       perf_evsel__object.fini(evsel);
> >  }
> >
> > --
> > 2.26.2.645.ge9eca65c58-goog
> >
>
> --
>
> - Arnaldo