* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.