* [PATCH] perf evsel: Fix memory leaks relating to unit
@ 2021-11-18 8:47 Ian Rogers
2021-11-18 13:19 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 2+ messages in thread
From: Ian Rogers @ 2021-11-18 8:47 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
linux-perf-users, linux-kernel
Cc: Stephane Eranian, Ian Rogers
unit may have a strdup pointer or be to a literal, consequently memory
assocciated with it isn't freed. Change it so the unit is always strdup
and so the memory can be safely freed. Fix related issue in
perf_event__process_event_update for name and own_cpus. Leaks were
spotted by leak sanitizer.
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/tests/event_update.c | 5 ++---
tools/perf/util/evsel.c | 18 +++++++++---------
| 8 +++++---
tools/perf/util/parse-events.c | 9 ++++++---
4 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/tools/perf/tests/event_update.c b/tools/perf/tests/event_update.c
index fbb68deba59f..d01532d40acb 100644
--- a/tools/perf/tests/event_update.c
+++ b/tools/perf/tests/event_update.c
@@ -88,7 +88,6 @@ static int test__event_update(struct test_suite *test __maybe_unused, int subtes
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);
@@ -99,7 +98,8 @@ static int test__event_update(struct test_suite *test __maybe_unused, int subtes
perf_evlist__id_add(&evlist->core, &evsel->core, 0, 0, 123);
- evsel->unit = unit;
+ free((char *)evsel->unit);
+ evsel->unit = strdup("KRAVA");
TEST_ASSERT_VAL("failed to synthesize attr update unit",
!perf_event__synthesize_event_update_unit(NULL, evsel, process_event_unit));
@@ -119,7 +119,6 @@ static int test__event_update(struct test_suite *test __maybe_unused, int subtes
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;
}
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index a59fb2ecb84e..ac0127be0459 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -241,7 +241,7 @@ void evsel__init(struct evsel *evsel,
{
perf_evsel__init(&evsel->core, attr, idx);
evsel->tracking = !idx;
- evsel->unit = "";
+ evsel->unit = strdup("");
evsel->scale = 1.0;
evsel->max_events = ULONG_MAX;
evsel->evlist = NULL;
@@ -276,13 +276,8 @@ struct evsel *evsel__new_idx(struct perf_event_attr *attr, int idx)
}
if (evsel__is_clock(evsel)) {
- /*
- * The evsel->unit points to static alias->unit
- * so it's ok to use static string in here.
- */
- static const char *unit = "msec";
-
- evsel->unit = unit;
+ free((char *)evsel->unit);
+ evsel->unit = strdup("msec");
evsel->scale = 1e-6;
}
@@ -420,7 +415,11 @@ struct evsel *evsel__clone(struct evsel *orig)
evsel->max_events = orig->max_events;
evsel->tool_event = orig->tool_event;
- evsel->unit = orig->unit;
+ free((char *)evsel->unit);
+ evsel->unit = strdup(orig->unit);
+ if (evsel->unit == NULL)
+ goto out_err;
+
evsel->scale = orig->scale;
evsel->snapshot = orig->snapshot;
evsel->per_pkg = orig->per_pkg;
@@ -1441,6 +1440,7 @@ void evsel__exit(struct evsel *evsel)
zfree(&evsel->group_name);
zfree(&evsel->name);
zfree(&evsel->pmu_name);
+ zfree(&evsel->unit);
zfree(&evsel->metric_id);
evsel__zero_per_pkg(evsel);
hashmap__free(evsel->per_pkg_mask);
--git a/tools/perf/util/header.c b/tools/perf/util/header.c
index fda8d14c891f..79cce216727e 100644
--- a/tools/perf/util/header.c
+++ b/tools/perf/util/header.c
@@ -4257,9 +4257,11 @@ int perf_event__process_event_update(struct perf_tool *tool __maybe_unused,
switch (ev->type) {
case PERF_EVENT_UPDATE__UNIT:
+ free((char *)evsel->unit);
evsel->unit = strdup(ev->data);
break;
case PERF_EVENT_UPDATE__NAME:
+ free(evsel->name);
evsel->name = strdup(ev->data);
break;
case PERF_EVENT_UPDATE__SCALE:
@@ -4268,11 +4270,11 @@ int perf_event__process_event_update(struct perf_tool *tool __maybe_unused,
break;
case PERF_EVENT_UPDATE__CPUS:
ev_cpus = (struct perf_record_event_update_cpus *)ev->data;
-
map = cpu_map__new_data(&ev_cpus->cpus);
- if (map)
+ if (map) {
+ perf_cpu_map__put(evsel->core.own_cpus);
evsel->core.own_cpus = map;
- else
+ } else
pr_err("failed to get event_update cpus\n");
default:
break;
diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
index a2f4c086986f..12d925a6d27f 100644
--- a/tools/perf/util/parse-events.c
+++ b/tools/perf/util/parse-events.c
@@ -402,8 +402,10 @@ static int add_event_tool(struct list_head *list, int *idx,
if (!evsel)
return -ENOMEM;
evsel->tool_event = tool_event;
- if (tool_event == PERF_TOOL_DURATION_TIME)
- evsel->unit = "ns";
+ if (tool_event == PERF_TOOL_DURATION_TIME) {
+ free((char *)evsel->unit);
+ evsel->unit = strdup("ns");
+ }
return 0;
}
@@ -1630,7 +1632,8 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
if (parse_state->fake_pmu)
return 0;
- evsel->unit = info.unit;
+ free((char *)evsel->unit);
+ evsel->unit = strdup(info.unit);
evsel->scale = info.scale;
evsel->per_pkg = info.per_pkg;
evsel->snapshot = info.snapshot;
--
2.34.0.rc1.387.gb447b232ab-goog
^ permalink raw reply related [flat|nested] 2+ messages in thread
* Re: [PATCH] perf evsel: Fix memory leaks relating to unit
2021-11-18 8:47 [PATCH] perf evsel: Fix memory leaks relating to unit Ian Rogers
@ 2021-11-18 13:19 ` Arnaldo Carvalho de Melo
0 siblings, 0 replies; 2+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-11-18 13:19 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, linux-perf-users, linux-kernel,
Stephane Eranian
Em Thu, Nov 18, 2021 at 12:47:49AM -0800, Ian Rogers escreveu:
> unit may have a strdup pointer or be to a literal, consequently memory
> assocciated with it isn't freed. Change it so the unit is always strdup
> and so the memory can be safely freed. Fix related issue in
> perf_event__process_event_update for name and own_cpus. Leaks were
> spotted by leak sanitizer.
Thanks, applied.
- Arnaldo
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/tests/event_update.c | 5 ++---
> tools/perf/util/evsel.c | 18 +++++++++---------
> tools/perf/util/header.c | 8 +++++---
> tools/perf/util/parse-events.c | 9 ++++++---
> 4 files changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/tools/perf/tests/event_update.c b/tools/perf/tests/event_update.c
> index fbb68deba59f..d01532d40acb 100644
> --- a/tools/perf/tests/event_update.c
> +++ b/tools/perf/tests/event_update.c
> @@ -88,7 +88,6 @@ static int test__event_update(struct test_suite *test __maybe_unused, int subtes
> 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);
>
> @@ -99,7 +98,8 @@ static int test__event_update(struct test_suite *test __maybe_unused, int subtes
>
> perf_evlist__id_add(&evlist->core, &evsel->core, 0, 0, 123);
>
> - evsel->unit = unit;
> + free((char *)evsel->unit);
> + evsel->unit = strdup("KRAVA");
>
> TEST_ASSERT_VAL("failed to synthesize attr update unit",
> !perf_event__synthesize_event_update_unit(NULL, evsel, process_event_unit));
> @@ -119,7 +119,6 @@ static int test__event_update(struct test_suite *test __maybe_unused, int subtes
> 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;
> }
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index a59fb2ecb84e..ac0127be0459 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -241,7 +241,7 @@ void evsel__init(struct evsel *evsel,
> {
> perf_evsel__init(&evsel->core, attr, idx);
> evsel->tracking = !idx;
> - evsel->unit = "";
> + evsel->unit = strdup("");
> evsel->scale = 1.0;
> evsel->max_events = ULONG_MAX;
> evsel->evlist = NULL;
> @@ -276,13 +276,8 @@ struct evsel *evsel__new_idx(struct perf_event_attr *attr, int idx)
> }
>
> if (evsel__is_clock(evsel)) {
> - /*
> - * The evsel->unit points to static alias->unit
> - * so it's ok to use static string in here.
> - */
> - static const char *unit = "msec";
> -
> - evsel->unit = unit;
> + free((char *)evsel->unit);
> + evsel->unit = strdup("msec");
> evsel->scale = 1e-6;
> }
>
> @@ -420,7 +415,11 @@ struct evsel *evsel__clone(struct evsel *orig)
>
> evsel->max_events = orig->max_events;
> evsel->tool_event = orig->tool_event;
> - evsel->unit = orig->unit;
> + free((char *)evsel->unit);
> + evsel->unit = strdup(orig->unit);
> + if (evsel->unit == NULL)
> + goto out_err;
> +
> evsel->scale = orig->scale;
> evsel->snapshot = orig->snapshot;
> evsel->per_pkg = orig->per_pkg;
> @@ -1441,6 +1440,7 @@ void evsel__exit(struct evsel *evsel)
> zfree(&evsel->group_name);
> zfree(&evsel->name);
> zfree(&evsel->pmu_name);
> + zfree(&evsel->unit);
> zfree(&evsel->metric_id);
> evsel__zero_per_pkg(evsel);
> hashmap__free(evsel->per_pkg_mask);
> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> index fda8d14c891f..79cce216727e 100644
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c
> @@ -4257,9 +4257,11 @@ int perf_event__process_event_update(struct perf_tool *tool __maybe_unused,
>
> switch (ev->type) {
> case PERF_EVENT_UPDATE__UNIT:
> + free((char *)evsel->unit);
> evsel->unit = strdup(ev->data);
> break;
> case PERF_EVENT_UPDATE__NAME:
> + free(evsel->name);
> evsel->name = strdup(ev->data);
> break;
> case PERF_EVENT_UPDATE__SCALE:
> @@ -4268,11 +4270,11 @@ int perf_event__process_event_update(struct perf_tool *tool __maybe_unused,
> break;
> case PERF_EVENT_UPDATE__CPUS:
> ev_cpus = (struct perf_record_event_update_cpus *)ev->data;
> -
> map = cpu_map__new_data(&ev_cpus->cpus);
> - if (map)
> + if (map) {
> + perf_cpu_map__put(evsel->core.own_cpus);
> evsel->core.own_cpus = map;
> - else
> + } else
> pr_err("failed to get event_update cpus\n");
> default:
> break;
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index a2f4c086986f..12d925a6d27f 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -402,8 +402,10 @@ static int add_event_tool(struct list_head *list, int *idx,
> if (!evsel)
> return -ENOMEM;
> evsel->tool_event = tool_event;
> - if (tool_event == PERF_TOOL_DURATION_TIME)
> - evsel->unit = "ns";
> + if (tool_event == PERF_TOOL_DURATION_TIME) {
> + free((char *)evsel->unit);
> + evsel->unit = strdup("ns");
> + }
> return 0;
> }
>
> @@ -1630,7 +1632,8 @@ int parse_events_add_pmu(struct parse_events_state *parse_state,
> if (parse_state->fake_pmu)
> return 0;
>
> - evsel->unit = info.unit;
> + free((char *)evsel->unit);
> + evsel->unit = strdup(info.unit);
> evsel->scale = info.scale;
> evsel->per_pkg = info.per_pkg;
> evsel->snapshot = info.snapshot;
> --
> 2.34.0.rc1.387.gb447b232ab-goog
--
- Arnaldo
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-11-18 13:19 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-18 8:47 [PATCH] perf evsel: Fix memory leaks relating to unit Ian Rogers
2021-11-18 13:19 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).