2021-07-15 16:55:20

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 00/20] perf: fix several memory leaks reported by ASan on perf-test

This patchset fixes several memory leaks found using ASan on perf-test.
After this patch, all memory leaks in perf-test that were found on my machine
are fixed.
This does not mean that there are no longer memory leaks in perf (just running
perf report or perf top raises a few), but that now it is possible to run the
perf tests with ASan to catch future memory errors.

While many of these leaks have little to no effect on the daily use of perf
(some were inside the test itself, other were just a missing cleanup on exit),
other were (slightly) more serious, namely:
- nsinfo refcounting was broken since it was not decreased when a reference was
replaced, causing many nsinfos to never being deallocated.
- dso__new_map refcounting was wrong, missing a __put
- lzma stream was never closed

Below are the results of perf-test before and after this patchset. Only failed
and skipped tests are reported for brevity.

The perf binary has been compiled using
make DEBUG=1 EXTRA_CFLAGS='-fno-omit-frame-pointer -fsanitize=address'
compilation using clang shows same results.

Tests are run on my laptop:
Intel(R) Core(TM) i7-5500U CPU @ 2.40GHz (2 cores + hyperthreading)
Fedora 34 w/ kernel 5.12.14-300.fc34.x86_64

Before this patchset:
1: vmlinux symtab matches kallsyms : FAILED!
19: 'import perf' in python : FAILED!
23.1: Read Only Watchpoint : Skip (missing hardware support)
31: Lookup mmap thread : FAILED!
41: Session topology : FAILED!
42.1: Basic BPF filtering : FAILED!
42.2: BPF pinning : Skip
42.3: BPF prologue generation : Skip
49: Synthesize attr update : FAILED!
58: builtin clang support : Skip (not compiled in)
63: Test libpfm4 support : Skip (not compiled in)
65: maps__merge_in : FAILED!
73: DWARF unwind : FAILED!
78: build id cache operations : FAILED!
82: Use vfs_getname probe to get syscall args filenames : FAILED!
83: Zstd perf.data compression/decompression : FAILED!
86: perf stat --bpf-counters test : Skip
87: Check Arm CoreSight trace data recording and synthesized samples: Skip
88: Check open filename arg using perf trace + vfs_getname : FAILED!

After this patchset:
19: 'import perf' in python : FAILED!
23.1: Read Only Watchpoint : Skip (missing hardware support)
58: builtin clang support : Skip (not compiled in)
63: Test libpfm4 support : Skip (not compiled in)
73: DWARF unwind : FAILED!
86: perf stat --bpf-counters test : Skip
87: Check Arm CoreSight trace data recording and synthesized samples: Skip

After this patchset, without ASan:
23.1: Read Only Watchpoint : Skip (missing hardware support)
58: builtin clang support : Skip (not compiled in)
63: Test libpfm4 support : Skip (not compiled in)
86: perf stat --bpf-counters test : Skip
87: Check Arm CoreSight trace data recording and synthesized samples: Skip

The two failing tests are false positives and can be skipped:
- "19: 'import perf' in python" fails since there are memory leaks inside the
python library.
- "73: DWARF unwind" fails due to stack buffer underflow, since it's unwinding
the stack.

Riccardo

Riccardo Mancini (20):
perf nsinfo: fix refcounting
perf env: fix sibling_dies memory leak
perf test: session_topology: delete session->evlist
perf test: event_update: fix memory leak of evlist
perf test: event_update: fix memory leak of unit
perf dso: fix memory leak in dso__new_map
perf test: maps__merge_in: fix memory leak of maps
perf env: fix memory leak of cpu_pmu_caps
perf report: free generated help strings for sort option
perf inject: close inject.output
perf session: cleanup trace_event
perf script: release zstd data
perf script: fix memory leaks in perf_script
perf util/lzma: close lzma stream
perf trace: free malloc'd trace fields on exit
perf trace: free syscall->arg_fmt
perf trace: free syscall tp fields in evsel->priv
perf trace: free strings in trace__parse_events_option
perf test: bpf: free obj_buf
perf util/probe-file: delete namelist on error in del_events

tools/perf/builtin-inject.c | 13 ++++++---
tools/perf/builtin-report.c | 33 ++++++++++++++-------
tools/perf/builtin-script.c | 8 ++++++
tools/perf/builtin-trace.c | 51 +++++++++++++++++++++++++++++++--
tools/perf/tests/bpf.c | 1 +
tools/perf/tests/event_update.c | 6 ++--
tools/perf/tests/maps.c | 2 ++
tools/perf/tests/topology.c | 1 +
tools/perf/util/dso.c | 4 ++-
tools/perf/util/env.c | 2 ++
tools/perf/util/lzma.c | 8 ++++--
tools/perf/util/map.c | 2 ++
tools/perf/util/probe-event.c | 4 ++-
tools/perf/util/probe-file.c | 4 +--
tools/perf/util/session.c | 1 +
tools/perf/util/sort.c | 2 +-
tools/perf/util/sort.h | 2 +-
17 files changed, 116 insertions(+), 28 deletions(-)

--
2.31.1


2021-07-15 16:55:28

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 20/20] perf util/probe-file: delete namelist on error in del_events

ASan reports some memory leaks when running the perf test
"42: BPF filter".
This second leak is caused by a strlist not being dellocated on error
inside probe_file__del_events.

This patch adds a goto label before the deallocation and makes the error
path jump to it.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/util/probe-file.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
index f9a6cbcd641501b8..3d50de3217d50ae1 100644
--- a/tools/perf/util/probe-file.c
+++ b/tools/perf/util/probe-file.c
@@ -377,11 +377,11 @@ int probe_file__del_events(int fd, struct strfilter *filter)

ret = probe_file__get_events(fd, filter, namelist);
if (ret < 0)
- return ret;
+ goto out;

ret = probe_file__del_strlist(fd, namelist);
+out:
strlist__delete(namelist);
-
return ret;
}

--
2.31.1

2021-07-15 16:55:30

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 14/20] perf util/lzma: close lzma stream

ASan reports memory leaks when running the perf test
"88: Check open filename arg using perf trace + vfs_getname".
One of these is caused by the lzma stream never being closed inside
lzma_decompress_to_file.

This patch adds the missing lzma_end.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/util/lzma.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/perf/util/lzma.c b/tools/perf/util/lzma.c
index 39062df0262915bd..51424cdc3b682c64 100644
--- a/tools/perf/util/lzma.c
+++ b/tools/perf/util/lzma.c
@@ -69,7 +69,7 @@ int lzma_decompress_to_file(const char *input, int output_fd)

if (ferror(infile)) {
pr_err("lzma: read error: %s\n", strerror(errno));
- goto err_fclose;
+ goto err_lzma_end;
}

if (feof(infile))
@@ -83,7 +83,7 @@ int lzma_decompress_to_file(const char *input, int output_fd)

if (writen(output_fd, buf_out, write_size) != write_size) {
pr_err("lzma: write error: %s\n", strerror(errno));
- goto err_fclose;
+ goto err_lzma_end;
}

strm.next_out = buf_out;
@@ -95,11 +95,13 @@ int lzma_decompress_to_file(const char *input, int output_fd)
break;

pr_err("lzma: failed %s\n", lzma_strerror(ret));
- goto err_fclose;
+ goto err_lzma_end;
}
}

err = 0;
+err_lzma_end:
+ lzma_end(&strm);
err_fclose:
fclose(infile);
return err;
--
2.31.1

2021-07-15 16:55:43

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 03/20] perf test: session_topology: delete session->evlist

ASan reports a memory leak related to session->evlist while running
the perf test "41: Session topology".

When perf_data is in write mode, session->evlist is owned by the
caller, which should also take care of deleting it.

This patch adds the missing evlist__delete.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/tests/topology.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
index ec4e3b21b8311f57..b5efe675b321746e 100644
--- a/tools/perf/tests/topology.c
+++ b/tools/perf/tests/topology.c
@@ -61,6 +61,7 @@ static int session_write_header(char *path)
TEST_ASSERT_VAL("failed to write header",
!perf_session__write_header(session, session->evlist, data.file.fd, true));

+ evlist__delete(session->evlist);
perf_session__delete(session);

return 0;
--
2.31.1

2021-07-15 16:56:11

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 19/20] perf test: bpf: free obj_buf

ASan reports some memory leaks when running the perf test
"42: BPF filter".
The first of these leaks is caused by obj_buf never being deallocated in
__test__bpf.

This patch adds the missing free.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/tests/bpf.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
index 33bda9c265423547..2cda4a6297e07967 100644
--- a/tools/perf/tests/bpf.c
+++ b/tools/perf/tests/bpf.c
@@ -276,6 +276,7 @@ static int __test__bpf(int idx)
}

out:
+ free(obj_buf);
bpf__clear();
return ret;
}
--
2.31.1

2021-07-15 16:57:15

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 09/20] perf report: free generated help strings for sort option

ASan reports the memory leak of the strings allocated by sort_help when
running perf report.

This patch changes the returned pointer to char* (instead of const char*),
saves it in a temporary variable, and finally deallocates it at function
exit.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/builtin-report.c | 33 ++++++++++++++++++++++-----------
tools/perf/util/sort.c | 2 +-
tools/perf/util/sort.h | 2 +-
3 files changed, 24 insertions(+), 13 deletions(-)

diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 6386af6a2612367c..dc0364f671b97d26 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -1175,6 +1175,8 @@ int cmd_report(int argc, const char **argv)
.annotation_opts = annotation__default_options,
.skip_empty = true,
};
+ char *sort_order_help = sort_help("sort by key(s):");
+ char *field_order_help = sort_help("output field(s): overhead period sample ");
const struct option options[] = {
OPT_STRING('i', "input", &input_name, "file",
"input file name"),
@@ -1209,9 +1211,9 @@ int cmd_report(int argc, const char **argv)
OPT_BOOLEAN(0, "header-only", &report.header_only,
"Show only data header."),
OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
- sort_help("sort by key(s):")),
+ sort_order_help),
OPT_STRING('F', "fields", &field_order, "key[,keys...]",
- sort_help("output field(s): overhead period sample ")),
+ field_order_help),
OPT_BOOLEAN(0, "show-cpu-utilization", &symbol_conf.show_cpu_utilization,
"Show sample percentage for different cpu modes"),
OPT_BOOLEAN_FLAG(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
@@ -1344,11 +1346,11 @@ int cmd_report(int argc, const char **argv)
char sort_tmp[128];

if (ret < 0)
- return ret;
+ goto exit;

ret = perf_config(report__config, &report);
if (ret)
- return ret;
+ goto exit;

argc = parse_options(argc, argv, options, report_usage, 0);
if (argc) {
@@ -1362,8 +1364,10 @@ int cmd_report(int argc, const char **argv)
report.symbol_filter_str = argv[0];
}

- if (annotate_check_args(&report.annotation_opts) < 0)
- return -EINVAL;
+ if (annotate_check_args(&report.annotation_opts) < 0) {
+ ret = -EINVAL;
+ goto exit;
+ }

if (report.mmaps_mode)
report.tasks_mode = true;
@@ -1377,12 +1381,14 @@ int cmd_report(int argc, const char **argv)
if (symbol_conf.vmlinux_name &&
access(symbol_conf.vmlinux_name, R_OK)) {
pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto exit;
}
if (symbol_conf.kallsyms_name &&
access(symbol_conf.kallsyms_name, R_OK)) {
pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto exit;
}

if (report.inverted_callchain)
@@ -1406,12 +1412,14 @@ int cmd_report(int argc, const char **argv)

repeat:
session = perf_session__new(&data, false, &report.tool);
- if (IS_ERR(session))
- return PTR_ERR(session);
+ if (IS_ERR(session)) {
+ ret = PTR_ERR(session);
+ goto exit;
+ }

ret = evswitch__init(&report.evswitch, session->evlist, stderr);
if (ret)
- return ret;
+ goto exit;

if (zstd_init(&(session->zstd_data), 0) < 0)
pr_warning("Decompression initialization failed. Reported data may be incomplete.\n");
@@ -1646,5 +1654,8 @@ int cmd_report(int argc, const char **argv)

zstd_fini(&(session->zstd_data));
perf_session__delete(session);
+exit:
+ free(sort_order_help);
+ free(field_order_help);
return ret;
}
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index 88ce47f2547e3558..568a88c001c6cb5a 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -3370,7 +3370,7 @@ static void add_hpp_sort_string(struct strbuf *sb, struct hpp_dimension *s, int
add_key(sb, s[i].name, llen);
}

-const char *sort_help(const char *prefix)
+char *sort_help(const char *prefix)
{
struct strbuf sb;
char *s;
diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
index 87a092645aa72e41..b67c469aba79587f 100644
--- a/tools/perf/util/sort.h
+++ b/tools/perf/util/sort.h
@@ -302,7 +302,7 @@ void reset_output_field(void);
void sort__setup_elide(FILE *fp);
void perf_hpp__set_elide(int idx, bool elide);

-const char *sort_help(const char *prefix);
+char *sort_help(const char *prefix);

int report_parse_ignore_callees_opt(const struct option *opt, const char *arg, int unset);

--
2.31.1

2021-07-15 16:57:50

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 18/20] perf trace: free strings in trace__parse_events_option

ASan reports several memory leaks running the perf test
"88: Check open filename arg using perf trace + vfs_getname".
The fourth of these leaks is related to some strings never being freed
in trace__parse_events_option.

This patch adds the missing frees.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/builtin-trace.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index d9c65d55a9ae7526..4a677f3ff273887a 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4659,6 +4659,9 @@ static int trace__parse_events_option(const struct option *opt, const char *str,
err = parse_events_option(&o, lists[0], 0);
}
out:
+ free(strace_groups_dir);
+ free(lists[0]);
+ free(lists[1]);
if (sep)
*sep = ',';

--
2.31.1

2021-07-15 18:13:50

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 07/20] perf test: maps__merge_in: fix memory leak of maps

ASan reports a memory leak when running the perf test
"65: maps__merge_in". This is the second and final patch addressing
these memory leaks.
This time, the problem is simply that the maps object is never
destructed.

This patch adds the missing maps__exit call.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/tests/maps.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/tools/perf/tests/maps.c b/tools/perf/tests/maps.c
index edcbc70ff9d66e22..1ac72919fa358601 100644
--- a/tools/perf/tests/maps.c
+++ b/tools/perf/tests/maps.c
@@ -116,5 +116,7 @@ int test__maps__merge_in(struct test *t __maybe_unused, int subtest __maybe_unus

ret = check_maps(merged3, ARRAY_SIZE(merged3), &maps);
TEST_ASSERT_VAL("merge check failed", !ret);
+
+ maps__exit(&maps);
return TEST_OK;
}
--
2.31.1

2021-07-15 18:13:51

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 06/20] perf dso: fix memory leak in dso__new_map

ASan reports a memory leak when running the perf test
"65: maps__merge_in". The causes of the leaks are two, this patch
addresses only the first one, which is related to dso__new_map.
The bug is that dso__new_map creates a new dso but never decreases
the refcount it gets from creating it.

This patch adds the missing dso__put.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/util/dso.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
index d786cf6b0cfa65f2..ee15db2be2f43403 100644
--- a/tools/perf/util/dso.c
+++ b/tools/perf/util/dso.c
@@ -1154,8 +1154,10 @@ struct map *dso__new_map(const char *name)
struct map *map = NULL;
struct dso *dso = dso__new(name);

- if (dso)
+ if (dso) {
map = map__new2(0, dso);
+ dso__put(dso);
+ }

return map;
}
--
2.31.1

2021-07-15 18:14:08

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 12/20] perf script: release zstd data

ASan reports several memory leak while running the perf test
"82: Use vfs_getname probe to get syscall args filenames".
One of the leaks is caused by zstd data not being released on exit in
perf-script.

This patch adds the missing zstd_fini.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/builtin-script.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index 8c03a9862872d495..bae0e5b72c0e6050 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -4143,6 +4143,7 @@ int cmd_script(int argc, const char **argv)
zfree(&script.ptime_range);
}

+ zstd_fini(&(session->zstd_data));
evlist__free_stats(session->evlist);
perf_session__delete(session);

--
2.31.1

2021-07-15 18:14:13

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 15/20] perf trace: free malloc'd trace fields on exit

ASan reports several memory leaks running the perf test
"88: Check open filename arg using perf trace + vfs_getname".
The first of these leaks is related to struct trace fields never being
deallocated.

This patch adds the function trace__exit, which is called at the end of
cmd_trace, replacing the existing deallocation, which is now moved
inside the new function.
This function deallocates:
- ev_qualifier
- ev_qualifier_ids.entries
- syscalls.table
- sctbl
- perfconfig_events

In order to prevent errors in case any of this field is never
initialized, this patch also adds initialization to NULL to these
fields.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/builtin-trace.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index 7ec18ff57fc4ae35..fe26d87ca8ccc14d 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -4701,6 +4701,15 @@ static int trace__config(const char *var, const char *value, void *arg)
return err;
}

+static void trace__exit(struct trace *trace)
+{
+ strlist__delete(trace->ev_qualifier);
+ free(trace->ev_qualifier_ids.entries);
+ free(trace->syscalls.table);
+ syscalltbl__delete(trace->sctbl);
+ zfree(&trace->perfconfig_events);
+}
+
int cmd_trace(int argc, const char **argv)
{
const char *trace_usage[] = {
@@ -4731,6 +4740,12 @@ int cmd_trace(int argc, const char **argv)
.kernel_syscallchains = false,
.max_stack = UINT_MAX,
.max_events = ULONG_MAX,
+ .ev_qualifier = NULL,
+ .sctbl = NULL,
+ .ev_qualifier_ids = {
+ .entries = NULL,
+ .nr = 0,
+ },
};
const char *map_dump_str = NULL;
const char *output_name = NULL;
@@ -5135,6 +5150,6 @@ int cmd_trace(int argc, const char **argv)
if (output_name != NULL)
fclose(trace.output);
out:
- zfree(&trace.perfconfig_events);
+ trace__exit(&trace);
return err;
}
--
2.31.1

2021-07-15 19:56:27

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 03/20] perf test: session_topology: delete session->evlist

Em Thu, Jul 15, 2021 at 06:07:08PM +0200, Riccardo Mancini escreveu:
> ASan reports a memory leak related to session->evlist while running
> the perf test "41: Session topology".
>
> When perf_data is in write mode, session->evlist is owned by the
> caller, which should also take care of deleting it.
>
> This patch adds the missing evlist__delete.

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/tests/topology.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/tests/topology.c b/tools/perf/tests/topology.c
> index ec4e3b21b8311f57..b5efe675b321746e 100644
> --- a/tools/perf/tests/topology.c
> +++ b/tools/perf/tests/topology.c
> @@ -61,6 +61,7 @@ static int session_write_header(char *path)
> TEST_ASSERT_VAL("failed to write header",
> !perf_session__write_header(session, session->evlist, data.file.fd, true));
>
> + evlist__delete(session->evlist);
> perf_session__delete(session);
>
> return 0;
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:03:40

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 06/20] perf dso: fix memory leak in dso__new_map

Em Thu, Jul 15, 2021 at 06:07:11PM +0200, Riccardo Mancini escreveu:
> ASan reports a memory leak when running the perf test
> "65: maps__merge_in". The causes of the leaks are two, this patch
> addresses only the first one, which is related to dso__new_map.
> The bug is that dso__new_map creates a new dso but never decreases
> the refcount it gets from creating it.
>
> This patch adds the missing dso__put.

Thanks, applied.

This needs some extra thinking for some unrelated issue, namely that
this dso isn't linked in the machine dso list, I think this is only used
by 'perf probe' :-\

I guess this won't be a problem since using the machine dso list is
important just for other tools, like when traversing to get the
build-ids, etc.

- Arnaldo

> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/util/dso.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index d786cf6b0cfa65f2..ee15db2be2f43403 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1154,8 +1154,10 @@ struct map *dso__new_map(const char *name)
> struct map *map = NULL;
> struct dso *dso = dso__new(name);
>
> - if (dso)
> + if (dso) {
> map = map__new2(0, dso);
> + dso__put(dso);
> + }
>
> return map;
> }
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:08:35

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 06/20] perf dso: fix memory leak in dso__new_map

Em Thu, Jul 15, 2021 at 06:07:11PM +0200, Riccardo Mancini escreveu:
> ASan reports a memory leak when running the perf test
> "65: maps__merge_in". The causes of the leaks are two, this patch
> addresses only the first one, which is related to dso__new_map.
> The bug is that dso__new_map creates a new dso but never decreases
> the refcount it gets from creating it.
>
> This patch adds the missing dso__put.

Fixes: d3a7c489c7fd2463 ("perf tools: Reference count struct dso")

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/util/dso.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
> index d786cf6b0cfa65f2..ee15db2be2f43403 100644
> --- a/tools/perf/util/dso.c
> +++ b/tools/perf/util/dso.c
> @@ -1154,8 +1154,10 @@ struct map *dso__new_map(const char *name)
> struct map *map = NULL;
> struct dso *dso = dso__new(name);
>
> - if (dso)
> + if (dso) {
> map = map__new2(0, dso);
> + dso__put(dso);
> + }
>
> return map;
> }
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:11:43

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 07/20] perf test: maps__merge_in: fix memory leak of maps

Em Thu, Jul 15, 2021 at 06:07:12PM +0200, Riccardo Mancini escreveu:
> ASan reports a memory leak when running the perf test
> "65: maps__merge_in". This is the second and final patch addressing
> these memory leaks.
> This time, the problem is simply that the maps object is never
> destructed.
>
> This patch adds the missing maps__exit call.

Not really this one, but at least the patch should apply more easily:

Fixes: 79b6bb73f888933c ("perf maps: Merge 'struct maps' with 'struct map_groups'")

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/tests/maps.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/tools/perf/tests/maps.c b/tools/perf/tests/maps.c
> index edcbc70ff9d66e22..1ac72919fa358601 100644
> --- a/tools/perf/tests/maps.c
> +++ b/tools/perf/tests/maps.c
> @@ -116,5 +116,7 @@ int test__maps__merge_in(struct test *t __maybe_unused, int subtest __maybe_unus
>
> ret = check_maps(merged3, ARRAY_SIZE(merged3), &maps);
> TEST_ASSERT_VAL("merge check failed", !ret);
> +
> + maps__exit(&maps);
> return TEST_OK;
> }
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:17:04

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 09/20] perf report: free generated help strings for sort option

Em Thu, Jul 15, 2021 at 06:07:14PM +0200, Riccardo Mancini escreveu:
> ASan reports the memory leak of the strings allocated by sort_help when
> running perf report.
>
> This patch changes the returned pointer to char* (instead of const char*),
> saves it in a temporary variable, and finally deallocates it at function
> exit.

Fixes: 702fb9b415e7c99b ("perf report: Show all sort keys in help output")

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/builtin-report.c | 33 ++++++++++++++++++++++-----------
> tools/perf/util/sort.c | 2 +-
> tools/perf/util/sort.h | 2 +-
> 3 files changed, 24 insertions(+), 13 deletions(-)
>
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 6386af6a2612367c..dc0364f671b97d26 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -1175,6 +1175,8 @@ int cmd_report(int argc, const char **argv)
> .annotation_opts = annotation__default_options,
> .skip_empty = true,
> };
> + char *sort_order_help = sort_help("sort by key(s):");
> + char *field_order_help = sort_help("output field(s): overhead period sample ");
> const struct option options[] = {
> OPT_STRING('i', "input", &input_name, "file",
> "input file name"),
> @@ -1209,9 +1211,9 @@ int cmd_report(int argc, const char **argv)
> OPT_BOOLEAN(0, "header-only", &report.header_only,
> "Show only data header."),
> OPT_STRING('s', "sort", &sort_order, "key[,key2...]",
> - sort_help("sort by key(s):")),
> + sort_order_help),
> OPT_STRING('F', "fields", &field_order, "key[,keys...]",
> - sort_help("output field(s): overhead period sample ")),
> + field_order_help),
> OPT_BOOLEAN(0, "show-cpu-utilization", &symbol_conf.show_cpu_utilization,
> "Show sample percentage for different cpu modes"),
> OPT_BOOLEAN_FLAG(0, "showcpuutilization", &symbol_conf.show_cpu_utilization,
> @@ -1344,11 +1346,11 @@ int cmd_report(int argc, const char **argv)
> char sort_tmp[128];
>
> if (ret < 0)
> - return ret;
> + goto exit;
>
> ret = perf_config(report__config, &report);
> if (ret)
> - return ret;
> + goto exit;
>
> argc = parse_options(argc, argv, options, report_usage, 0);
> if (argc) {
> @@ -1362,8 +1364,10 @@ int cmd_report(int argc, const char **argv)
> report.symbol_filter_str = argv[0];
> }
>
> - if (annotate_check_args(&report.annotation_opts) < 0)
> - return -EINVAL;
> + if (annotate_check_args(&report.annotation_opts) < 0) {
> + ret = -EINVAL;
> + goto exit;
> + }
>
> if (report.mmaps_mode)
> report.tasks_mode = true;
> @@ -1377,12 +1381,14 @@ int cmd_report(int argc, const char **argv)
> if (symbol_conf.vmlinux_name &&
> access(symbol_conf.vmlinux_name, R_OK)) {
> pr_err("Invalid file: %s\n", symbol_conf.vmlinux_name);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto exit;
> }
> if (symbol_conf.kallsyms_name &&
> access(symbol_conf.kallsyms_name, R_OK)) {
> pr_err("Invalid file: %s\n", symbol_conf.kallsyms_name);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto exit;
> }
>
> if (report.inverted_callchain)
> @@ -1406,12 +1412,14 @@ int cmd_report(int argc, const char **argv)
>
> repeat:
> session = perf_session__new(&data, false, &report.tool);
> - if (IS_ERR(session))
> - return PTR_ERR(session);
> + if (IS_ERR(session)) {
> + ret = PTR_ERR(session);
> + goto exit;
> + }
>
> ret = evswitch__init(&report.evswitch, session->evlist, stderr);
> if (ret)
> - return ret;
> + goto exit;
>
> if (zstd_init(&(session->zstd_data), 0) < 0)
> pr_warning("Decompression initialization failed. Reported data may be incomplete.\n");
> @@ -1646,5 +1654,8 @@ int cmd_report(int argc, const char **argv)
>
> zstd_fini(&(session->zstd_data));
> perf_session__delete(session);
> +exit:
> + free(sort_order_help);
> + free(field_order_help);
> return ret;
> }
> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 88ce47f2547e3558..568a88c001c6cb5a 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
> @@ -3370,7 +3370,7 @@ static void add_hpp_sort_string(struct strbuf *sb, struct hpp_dimension *s, int
> add_key(sb, s[i].name, llen);
> }
>
> -const char *sort_help(const char *prefix)
> +char *sort_help(const char *prefix)
> {
> struct strbuf sb;
> char *s;
> diff --git a/tools/perf/util/sort.h b/tools/perf/util/sort.h
> index 87a092645aa72e41..b67c469aba79587f 100644
> --- a/tools/perf/util/sort.h
> +++ b/tools/perf/util/sort.h
> @@ -302,7 +302,7 @@ void reset_output_field(void);
> void sort__setup_elide(FILE *fp);
> void perf_hpp__set_elide(int idx, bool elide);
>
> -const char *sort_help(const char *prefix);
> +char *sort_help(const char *prefix);
>
> int report_parse_ignore_callees_opt(const struct option *opt, const char *arg, int unset);
>
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:24:32

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 12/20] perf script: release zstd data

Em Thu, Jul 15, 2021 at 06:07:17PM +0200, Riccardo Mancini escreveu:
> ASan reports several memory leak while running the perf test
> "82: Use vfs_getname probe to get syscall args filenames".
> One of the leaks is caused by zstd data not being released on exit in
> perf-script.
>
> This patch adds the missing zstd_fini.

Fixes: b13b04d9382113f7 ("perf script: Initialize zstd_data")

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/builtin-script.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> index 8c03a9862872d495..bae0e5b72c0e6050 100644
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c
> @@ -4143,6 +4143,7 @@ int cmd_script(int argc, const char **argv)
> zfree(&script.ptime_range);
> }
>
> + zstd_fini(&(session->zstd_data));
> evlist__free_stats(session->evlist);
> perf_session__delete(session);
>
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:33:18

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 14/20] perf util/lzma: close lzma stream

Em Thu, Jul 15, 2021 at 06:07:19PM +0200, Riccardo Mancini escreveu:
> ASan reports memory leaks when running the perf test
> "88: Check open filename arg using perf trace + vfs_getname".
> One of these is caused by the lzma stream never being closed inside
> lzma_decompress_to_file.
>
> This patch adds the missing lzma_end.

Fixes: 80a32e5b498a7547 ("perf tools: Add lzma decompression support for kernel module")

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/util/lzma.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/lzma.c b/tools/perf/util/lzma.c
> index 39062df0262915bd..51424cdc3b682c64 100644
> --- a/tools/perf/util/lzma.c
> +++ b/tools/perf/util/lzma.c
> @@ -69,7 +69,7 @@ int lzma_decompress_to_file(const char *input, int output_fd)
>
> if (ferror(infile)) {
> pr_err("lzma: read error: %s\n", strerror(errno));
> - goto err_fclose;
> + goto err_lzma_end;
> }
>
> if (feof(infile))
> @@ -83,7 +83,7 @@ int lzma_decompress_to_file(const char *input, int output_fd)
>
> if (writen(output_fd, buf_out, write_size) != write_size) {
> pr_err("lzma: write error: %s\n", strerror(errno));
> - goto err_fclose;
> + goto err_lzma_end;
> }
>
> strm.next_out = buf_out;
> @@ -95,11 +95,13 @@ int lzma_decompress_to_file(const char *input, int output_fd)
> break;
>
> pr_err("lzma: failed %s\n", lzma_strerror(ret));
> - goto err_fclose;
> + goto err_lzma_end;
> }
> }
>
> err = 0;
> +err_lzma_end:
> + lzma_end(&strm);
> err_fclose:
> fclose(infile);
> return err;
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:35:55

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 15/20] perf trace: free malloc'd trace fields on exit

Em Thu, Jul 15, 2021 at 06:07:20PM +0200, Riccardo Mancini escreveu:
> ASan reports several memory leaks running the perf test
> "88: Check open filename arg using perf trace + vfs_getname".
> The first of these leaks is related to struct trace fields never being
> deallocated.
>
> This patch adds the function trace__exit, which is called at the end of
> cmd_trace, replacing the existing deallocation, which is now moved
> inside the new function.
> This function deallocates:
> - ev_qualifier
> - ev_qualifier_ids.entries
> - syscalls.table
> - sctbl
> - perfconfig_events
>
> In order to prevent errors in case any of this field is never
> initialized, this patch also adds initialization to NULL to these
> fields.
>
> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/builtin-trace.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index 7ec18ff57fc4ae35..fe26d87ca8ccc14d 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -4701,6 +4701,15 @@ static int trace__config(const char *var, const char *value, void *arg)
> return err;
> }
>
> +static void trace__exit(struct trace *trace)
> +{
> + strlist__delete(trace->ev_qualifier);
> + free(trace->ev_qualifier_ids.entries);
> + free(trace->syscalls.table);
> + syscalltbl__delete(trace->sctbl);
> + zfree(&trace->perfconfig_events);
> +}
> +
> int cmd_trace(int argc, const char **argv)
> {
> const char *trace_usage[] = {
> @@ -4731,6 +4740,12 @@ int cmd_trace(int argc, const char **argv)
> .kernel_syscallchains = false,
> .max_stack = UINT_MAX,
> .max_events = ULONG_MAX,
> + .ev_qualifier = NULL,
> + .sctbl = NULL,
> + .ev_qualifier_ids = {
> + .entries = NULL,
> + .nr = 0,
> + },
> };
> const char *map_dump_str = NULL;
> const char *output_name = NULL;

The above hunk is not needed, as all non explicitely initialized members
will be zeroed, so to reduce patch size I'm dropping it.

Thanks, applied.

- Arnaldo


> @@ -5135,6 +5150,6 @@ int cmd_trace(int argc, const char **argv)
> if (output_name != NULL)
> fclose(trace.output);
> out:
> - zfree(&trace.perfconfig_events);
> + trace__exit(&trace);
> return err;
> }
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:37:13

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 18/20] perf trace: free strings in trace__parse_events_option

Em Thu, Jul 15, 2021 at 06:07:23PM +0200, Riccardo Mancini escreveu:
> ASan reports several memory leaks running the perf test
> "88: Check open filename arg using perf trace + vfs_getname".
> The fourth of these leaks is related to some strings never being freed
> in trace__parse_events_option.
>
> This patch adds the missing frees.

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/builtin-trace.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
> index d9c65d55a9ae7526..4a677f3ff273887a 100644
> --- a/tools/perf/builtin-trace.c
> +++ b/tools/perf/builtin-trace.c
> @@ -4659,6 +4659,9 @@ static int trace__parse_events_option(const struct option *opt, const char *str,
> err = parse_events_option(&o, lists[0], 0);
> }
> out:
> + free(strace_groups_dir);
> + free(lists[0]);
> + free(lists[1]);
> if (sep)
> *sep = ',';
>
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:41:32

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 19/20] perf test: bpf: free obj_buf

Em Thu, Jul 15, 2021 at 06:07:24PM +0200, Riccardo Mancini escreveu:
> ASan reports some memory leaks when running the perf test
> "42: BPF filter".
> The first of these leaks is caused by obj_buf never being deallocated in
> __test__bpf.
>
> This patch adds the missing free.

Fixes: ba1fae431e74bb42 ("perf test: Add 'perf test BPF'")

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/tests/bpf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
> index 33bda9c265423547..2cda4a6297e07967 100644
> --- a/tools/perf/tests/bpf.c
> +++ b/tools/perf/tests/bpf.c
> @@ -276,6 +276,7 @@ static int __test__bpf(int idx)
> }
>
> out:
> + free(obj_buf);
> bpf__clear();
> return ret;
> }
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 20:43:08

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 20/20] perf util/probe-file: delete namelist on error in del_events

Em Thu, Jul 15, 2021 at 06:07:25PM +0200, Riccardo Mancini escreveu:
> ASan reports some memory leaks when running the perf test
> "42: BPF filter".
> This second leak is caused by a strlist not being dellocated on error
> inside probe_file__del_events.
>
> This patch adds a goto label before the deallocation and makes the error
> path jump to it.

Fixes: e7895e422e4da63d ("perf probe: Split del_perf_probe_events()")

Thanks, applied.

- Arnaldo


> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/util/probe-file.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/probe-file.c b/tools/perf/util/probe-file.c
> index f9a6cbcd641501b8..3d50de3217d50ae1 100644
> --- a/tools/perf/util/probe-file.c
> +++ b/tools/perf/util/probe-file.c
> @@ -377,11 +377,11 @@ int probe_file__del_events(int fd, struct strfilter *filter)
>
> ret = probe_file__get_events(fd, filter, namelist);
> if (ret < 0)
> - return ret;
> + goto out;
>
> ret = probe_file__del_strlist(fd, namelist);
> +out:
> strlist__delete(namelist);
> -
> return ret;
> }
>
> --
> 2.31.1
>

--

- Arnaldo

2021-07-15 21:00:30

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 01/20] perf nsinfo: fix refcounting

ASan reports a memory leak of nsinfo during the execution of the perf
test "31: Lookup mmap thread".
The leak is caused by a refcounted variable being replaced without
dropping the refcount.

This patch makes sure that the refcnt of nsinfo is decreased whenever
a refcounted variable is replaced with a new value.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/builtin-inject.c | 5 +++--
tools/perf/util/map.c | 2 ++
tools/perf/util/probe-event.c | 4 +++-
3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
index 5d6f583e2cd35be0..ffd2b25039e36e1d 100644
--- a/tools/perf/builtin-inject.c
+++ b/tools/perf/builtin-inject.c
@@ -361,9 +361,10 @@ static struct dso *findnew_dso(int pid, int tid, const char *filename,
dso = machine__findnew_dso_id(machine, filename, id);
}

- if (dso)
+ if (dso) {
+ nsinfo__put(dso->nsinfo);
dso->nsinfo = nsi;
- else
+ } else
nsinfo__put(nsi);

thread__put(thread);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 8af693d9678cefe0..72e7f3616157ead4 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -192,6 +192,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
if (!(prot & PROT_EXEC))
dso__set_loaded(dso);
}
+
+ nsinfo__put(dso->nsinfo);
dso->nsinfo = nsi;

if (build_id__is_defined(bid))
diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
index c14e1d228e56b1c6..e05750cea34c3a95 100644
--- a/tools/perf/util/probe-event.c
+++ b/tools/perf/util/probe-event.c
@@ -179,8 +179,10 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user)
struct map *map;

map = dso__new_map(target);
- if (map && map->dso)
+ if (map && map->dso) {
+ nsinfo__put(map->dso->nsinfo);
map->dso->nsinfo = nsinfo__get(nsi);
+ }
return map;
} else {
return kernel_get_module_map(target);
--
2.31.1

2021-07-15 21:00:32

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 02/20] perf env: fix sibling_dies memory leak

ASan reports a memory leak in perf_env while running the perf test
"41: Session topology", caused by sibling_dies not being freed.

This patch adds the required free.

Fixes: acae8b36cded0ee6 ("perf header: Add die information in CPU topology")
Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/util/env.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/env.c b/tools/perf/util/env.c
index ebc5e9ad35db21d1..6c765946ef6f591c 100644
--- a/tools/perf/util/env.c
+++ b/tools/perf/util/env.c
@@ -186,6 +186,7 @@ void perf_env__exit(struct perf_env *env)
zfree(&env->cpuid);
zfree(&env->cmdline);
zfree(&env->cmdline_argv);
+ zfree(&env->sibling_dies);
zfree(&env->sibling_cores);
zfree(&env->sibling_threads);
zfree(&env->pmu_mappings);
--
2.31.1

2021-07-15 21:00:35

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 04/20] perf test: event_update: fix memory leak of evlist

ASan reports a memory leak when running the perf test
"49: Synthesize attr update" caused by evlist not being deleted.

This patch adds the missing evlist__delete and removes the
perf_cpu_map__put since it's already being deleted by evlist__delete.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/tests/event_update.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/tests/event_update.c b/tools/perf/tests/event_update.c
index 656218179222cc58..932ab0740d11ced6 100644
--- a/tools/perf/tests/event_update.c
+++ b/tools/perf/tests/event_update.c
@@ -118,6 +118,6 @@ int test__event_update(struct test *test __maybe_unused, int subtest __maybe_unu
TEST_ASSERT_VAL("failed to synthesize attr update cpus",
!perf_event__synthesize_event_update_cpus(&tmp.tool, evsel, process_event_cpus));

- perf_cpu_map__put(evsel->core.own_cpus);
+ evlist__delete(evlist);
return 0;
}
--
2.31.1

2021-07-15 21:00:47

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 13/20] perf script: fix memory leaks in perf_script

ASan reports several memory leaks while running the perf test
"82: Use vfs_getname probe to get syscall args filenames".
Two of these are caused by some refcounts not being decreased on
perf-script exit, namely script.threads and script.cpus.

This patch adds the missing __put calls in a new perf_script__exit
function, which is called at the end of cmd_script.

This patch concludes the fixes of all remaining memory leaks in perf
test "82: Use vfs_getname probe to get syscall args filenames".

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/builtin-script.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
index bae0e5b72c0e6050..064da7f3618d39d8 100644
--- a/tools/perf/builtin-script.c
+++ b/tools/perf/builtin-script.c
@@ -2601,6 +2601,12 @@ static void perf_script__exit_per_event_dump_stats(struct perf_script *script)
}
}

+static void perf_script__exit(struct perf_script *script)
+{
+ perf_thread_map__put(script->threads);
+ perf_cpu_map__put(script->cpus);
+}
+
static int __cmd_script(struct perf_script *script)
{
int ret;
@@ -4146,6 +4152,7 @@ int cmd_script(int argc, const char **argv)
zstd_fini(&(session->zstd_data));
evlist__free_stats(session->evlist);
perf_session__delete(session);
+ perf_script__exit(&script);

if (script_started)
cleanup_scripting();
--
2.31.1

2021-07-15 21:01:49

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 05/20] perf test: event_update: fix memory leak of unit

ASan reports a memory leak while running the perf test
"49: Synthesize attr update" caused by a string being duplicated but
never freed.

This patch adds the missing free().

Note that evsel->unit is not deallocated together with evsel since it
is supposed to be a constant string.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/tests/event_update.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/tests/event_update.c b/tools/perf/tests/event_update.c
index 932ab0740d11ced6..44a50527f9d95170 100644
--- a/tools/perf/tests/event_update.c
+++ b/tools/perf/tests/event_update.c
@@ -88,6 +88,7 @@ int test__event_update(struct test *test __maybe_unused, int subtest __maybe_unu
struct evsel *evsel;
struct event_name tmp;
struct evlist *evlist = evlist__new_default();
+ char *unit = strdup("KRAVA");

TEST_ASSERT_VAL("failed to get evlist", evlist);

@@ -98,7 +99,7 @@ int test__event_update(struct test *test __maybe_unused, int subtest __maybe_unu

perf_evlist__id_add(&evlist->core, &evsel->core, 0, 0, 123);

- evsel->unit = strdup("KRAVA");
+ evsel->unit = unit;

TEST_ASSERT_VAL("failed to synthesize attr update unit",
!perf_event__synthesize_event_update_unit(NULL, evsel, process_event_unit));
@@ -118,6 +119,7 @@ int test__event_update(struct test *test __maybe_unused, int subtest __maybe_unu
TEST_ASSERT_VAL("failed to synthesize attr update cpus",
!perf_event__synthesize_event_update_cpus(&tmp.tool, evsel, process_event_cpus));

+ free(unit);
evlist__delete(evlist);
return 0;
}
--
2.31.1

2021-07-15 21:02:05

by Riccardo Mancini

[permalink] [raw]
Subject: [PATCH 17/20] perf trace: free syscall tp fields in evsel->priv

ASan reports several memory leaks running the perf test
"88: Check open filename arg using perf trace + vfs_getname".
The third of these leaks is related to evsel->priv fields of sycalls never
being deallocated.

This patch adds the function evlist__free_syscall_tp_fields which
iterates over all evsels in evlist, matching syscalls, and calling the
missing frees.
This new function is called at the end of trace__run, right before
calling evlist__delete.

Signed-off-by: Riccardo Mancini <[email protected]>
---
tools/perf/builtin-trace.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index d67f02d237eb0c7e..d9c65d55a9ae7526 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -3103,6 +3103,21 @@ static struct evsel *evsel__new_pgfault(u64 config)
return evsel;
}

+static void evlist__free_syscall_tp_fields(struct evlist *evlist)
+{
+ struct evsel *evsel;
+
+ evlist__for_each_entry(evlist, evsel) {
+ struct evsel_trace *et = evsel->priv;
+
+ if (!et || !evsel->tp_format || strcmp(evsel->tp_format->system, "syscalls"))
+ continue;
+
+ free(et->fmt);
+ free(et);
+ }
+}
+
static void trace__handle_event(struct trace *trace, union perf_event *event, struct perf_sample *sample)
{
const u32 type = event->header.type;
@@ -4138,7 +4153,7 @@ static int trace__run(struct trace *trace, int argc, const char **argv)

out_delete_evlist:
trace__symbols__exit(trace);
-
+ evlist__free_syscall_tp_fields(evlist);
evlist__delete(evlist);
cgroup__put(trace->cgroup);
trace->evlist = NULL;
--
2.31.1

2021-07-15 21:11:48

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 01/20] perf nsinfo: fix refcounting

Em Thu, Jul 15, 2021 at 06:07:06PM +0200, Riccardo Mancini escreveu:
> ASan reports a memory leak of nsinfo during the execution of the perf
> test "31: Lookup mmap thread".
> The leak is caused by a refcounted variable being replaced without
> dropping the refcount.
>
> This patch makes sure that the refcnt of nsinfo is decreased whenever
> a refcounted variable is replaced with a new value.

So, there are multiple fixes in just one patch, I'll split it into
three, no need to resend.

I'll try and check if finding Fixes: for the three is easy, that way
[email protected] will figure out which of the supported releases
need each of them.

- Arnaldo

> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/builtin-inject.c | 5 +++--
> tools/perf/util/map.c | 2 ++
> tools/perf/util/probe-event.c | 4 +++-
> 3 files changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/builtin-inject.c b/tools/perf/builtin-inject.c
> index 5d6f583e2cd35be0..ffd2b25039e36e1d 100644
> --- a/tools/perf/builtin-inject.c
> +++ b/tools/perf/builtin-inject.c
> @@ -361,9 +361,10 @@ static struct dso *findnew_dso(int pid, int tid, const char *filename,
> dso = machine__findnew_dso_id(machine, filename, id);
> }
>
> - if (dso)
> + if (dso) {
> + nsinfo__put(dso->nsinfo);
> dso->nsinfo = nsi;
> - else
> + } else
> nsinfo__put(nsi);
>
> thread__put(thread);
> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
> index 8af693d9678cefe0..72e7f3616157ead4 100644
> --- a/tools/perf/util/map.c
> +++ b/tools/perf/util/map.c
> @@ -192,6 +192,8 @@ struct map *map__new(struct machine *machine, u64 start, u64 len,
> if (!(prot & PROT_EXEC))
> dso__set_loaded(dso);
> }
> +
> + nsinfo__put(dso->nsinfo);
> dso->nsinfo = nsi;
>
> if (build_id__is_defined(bid))
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index c14e1d228e56b1c6..e05750cea34c3a95 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -179,8 +179,10 @@ struct map *get_target_map(const char *target, struct nsinfo *nsi, bool user)
> struct map *map;
>
> map = dso__new_map(target);
> - if (map && map->dso)
> + if (map && map->dso) {
> + nsinfo__put(map->dso->nsinfo);
> map->dso->nsinfo = nsinfo__get(nsi);
> + }
> return map;
> } else {
> return kernel_get_module_map(target);
> --
> 2.31.1
>

--

- Arnaldo

2021-07-16 16:54:09

by Arnaldo Carvalho de Melo

[permalink] [raw]
Subject: Re: [PATCH 19/20] perf test: bpf: free obj_buf

Em Thu, Jul 15, 2021 at 06:07:24PM +0200, Riccardo Mancini escreveu:
> ASan reports some memory leaks when running the perf test
> "42: BPF filter".
> The first of these leaks is caused by obj_buf never being deallocated in
> __test__bpf.
>
> This patch adds the missing free.
>
> Signed-off-by: Riccardo Mancini <[email protected]>
> ---
> tools/perf/tests/bpf.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/tools/perf/tests/bpf.c b/tools/perf/tests/bpf.c
> index 33bda9c265423547..2cda4a6297e07967 100644
> --- a/tools/perf/tests/bpf.c
> +++ b/tools/perf/tests/bpf.c
> @@ -276,6 +276,7 @@ static int __test__bpf(int idx)
> }
>
> out:
> + free(obj_buf);
> bpf__clear();
> return ret;
> }
> --

I followed the advice and added the stdlib.h include, elsewhere we're
getting included via some other include by luck.

- Arnaldo

66 6.97 ubuntu:18.04-x-m68k : FAIL gcc version 7.5.0 (Ubuntu 7.5.0-3ubuntu1~18.04)
tests/bpf.c: In function '__test__bpf':
tests/bpf.c:279:2: error: implicit declaration of function 'free' [-Werror=implicit-function-declaration]
free(obj_buf);
^~~~
tests/bpf.c:279:2: error: incompatible implicit declaration of built-in function 'free' [-Werror]
tests/bpf.c:279:2: note: include '<stdlib.h>' or provide a declaration of 'free'
cc1: all warnings being treated as errors
/git/perf-5.14.0-rc1/tools/build/Makefile.build:139: recipe for target 'tests' failed
make[3]: *** [tests] Error 2