* [PATCH] perf metric: Fix memory leaks.
@ 2021-11-05 16:46 Ian Rogers
2021-11-06 19:51 ` Arnaldo Carvalho de Melo
0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2021-11-05 16:46 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Mark Rutland, Alexander Shishkin, Jiri Olsa, Namhyung Kim,
John Garry, linux-perf-users, linux-kernel
Cc: eranian, Ian Rogers
Certain error paths may leak memory as caught by address sanitizer.
Ensure this is cleaned up to make sure address/leak sanitizer is happy.
Fixes: 5ecd5a0c7d1c ("perf metrics: Modify setup and deduplication")
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/util/metricgroup.c | 13 ++++++++++++-
1 file changed, 12 insertions(+), 1 deletion(-)
diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
index 4917e9704765..734d2ce94825 100644
--- a/tools/perf/util/metricgroup.c
+++ b/tools/perf/util/metricgroup.c
@@ -228,6 +228,7 @@ static void metric__free(struct metric *m)
free(m->metric_refs);
expr__ctx_free(m->pctx);
free((char *)m->modifier);
+ evlist__delete(m->evlist);
free(m);
}
@@ -1352,6 +1353,14 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
*out_evlist = parsed_evlist;
parsed_evlist = NULL;
err_out:
+ /*
+ * Errors are generally cleaned up by printing, but parsing may succeed
+ * with intermediate unused errors being recorded.
+ */
+ free(parse_error.str);
+ free(parse_error.help);
+ free(parse_error.first_str);
+ free(parse_error.first_help);
evlist__delete(parsed_evlist);
strbuf_release(&events);
return ret;
@@ -1481,8 +1490,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
}
- if (combined_evlist)
+ if (combined_evlist) {
evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
+ evlist__delete(combined_evlist);
+ }
list_for_each_entry(m, &metric_list, nd) {
if (m->evlist)
--
2.34.0.rc0.344.g81b53c2807-goog
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] perf metric: Fix memory leaks.
2021-11-05 16:46 [PATCH] perf metric: Fix memory leaks Ian Rogers
@ 2021-11-06 19:51 ` Arnaldo Carvalho de Melo
[not found] ` <CAP-5=fVMtO6zd_Qm9rJkMNGB==606hW-PRiQ7VCWFJLpJRmyBQ@mail.gmail.com>
0 siblings, 1 reply; 3+ messages in thread
From: Arnaldo Carvalho de Melo @ 2021-11-06 19:51 UTC (permalink / raw)
To: Ian Rogers
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, John Garry, linux-perf-users,
linux-kernel, eranian
Em Fri, Nov 05, 2021 at 09:46:57AM -0700, Ian Rogers escreveu:
> Certain error paths may leak memory as caught by address sanitizer.
> Ensure this is cleaned up to make sure address/leak sanitizer is happy.
>
> Fixes: 5ecd5a0c7d1c ("perf metrics: Modify setup and deduplication")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
> tools/perf/util/metricgroup.c | 13 ++++++++++++-
> 1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 4917e9704765..734d2ce94825 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c
> @@ -228,6 +228,7 @@ static void metric__free(struct metric *m)
> free(m->metric_refs);
> expr__ctx_free(m->pctx);
> free((char *)m->modifier);
> + evlist__delete(m->evlist);
> free(m);
> }
>
> @@ -1352,6 +1353,14 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
> *out_evlist = parsed_evlist;
> parsed_evlist = NULL;
> err_out:
> + /*
> + * Errors are generally cleaned up by printing, but parsing may succeed
> + * with intermediate unused errors being recorded.
> + */
> + free(parse_error.str);
> + free(parse_error.help);
> + free(parse_error.first_str);
> + free(parse_error.first_help);
Can't this be in a parse_events__free_errors() routine?
- Arnaldo
> evlist__delete(parsed_evlist);
> strbuf_release(&events);
> return ret;
> @@ -1481,8 +1490,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
> }
>
>
> - if (combined_evlist)
> + if (combined_evlist) {
> evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
> + evlist__delete(combined_evlist);
> + }
>
> list_for_each_entry(m, &metric_list, nd) {
> if (m->evlist)
> --
> 2.34.0.rc0.344.g81b53c2807-goog
--
- Arnaldo
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] perf metric: Fix memory leaks.
[not found] ` <CAP-5=fVMtO6zd_Qm9rJkMNGB==606hW-PRiQ7VCWFJLpJRmyBQ@mail.gmail.com>
@ 2021-11-07 9:05 ` Ian Rogers
0 siblings, 0 replies; 3+ messages in thread
From: Ian Rogers @ 2021-11-07 9:05 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin,
Jiri Olsa, Namhyung Kim, John Garry, linux-perf-users, LKML,
Stephane Eranian
On Sat, Nov 6, 2021 at 5:36 PM Ian Rogers <irogers@google.com> wrote:
>
>
>
> On Sat, Nov 6, 2021, 12:51 PM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>>
>> Em Fri, Nov 05, 2021 at 09:46:57AM -0700, Ian Rogers escreveu:
>> > Certain error paths may leak memory as caught by address sanitizer.
>> > Ensure this is cleaned up to make sure address/leak sanitizer is happy.
>> >
>> > Fixes: 5ecd5a0c7d1c ("perf metrics: Modify setup and deduplication")
>> > Signed-off-by: Ian Rogers <irogers@google.com>
>> > ---
>> > tools/perf/util/metricgroup.c | 13 ++++++++++++-
>> > 1 file changed, 12 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
>> > index 4917e9704765..734d2ce94825 100644
>> > --- a/tools/perf/util/metricgroup.c
>> > +++ b/tools/perf/util/metricgroup.c
>> > @@ -228,6 +228,7 @@ static void metric__free(struct metric *m)
>> > free(m->metric_refs);
>> > expr__ctx_free(m->pctx);
>> > free((char *)m->modifier);
>> > + evlist__delete(m->evlist);
>> > free(m);
>> > }
>> >
>> > @@ -1352,6 +1353,14 @@ static int parse_ids(struct perf_pmu *fake_pmu, struct expr_parse_ctx *ids,
>> > *out_evlist = parsed_evlist;
>> > parsed_evlist = NULL;
>> > err_out:
>> > + /*
>> > + * Errors are generally cleaned up by printing, but parsing may succeed
>> > + * with intermediate unused errors being recorded.
>> > + */
>> > + free(parse_error.str);
>> > + free(parse_error.help);
>> > + free(parse_error.first_str);
>> > + free(parse_error.first_help);
>>
>> Can't this be in a parse_events__free_errors() routine?
>
>
> I was wondering about an init and exit routine, I'm not sure on the convention here. The code currently assumes that an error will cause printing and the print routine frees. It is possible to have events that fail but overall parsing succeeds. We need to free in both cases which currently isn't done with a helper.
>
> Thanks,
> Ian
I did the bigger cleanup with init/exit in v2:
https://lore.kernel.org/lkml/20211107090002.3784612-2-irogers@google.com/
Thanks,
Ian
>
>
>> - Arnaldo
>>
>> > evlist__delete(parsed_evlist);
>> > strbuf_release(&events);
>> > return ret;
>> > @@ -1481,8 +1490,10 @@ static int parse_groups(struct evlist *perf_evlist, const char *str,
>> > }
>> >
>> >
>> > - if (combined_evlist)
>> > + if (combined_evlist) {
>> > evlist__splice_list_tail(perf_evlist, &combined_evlist->core.entries);
>> > + evlist__delete(combined_evlist);
>> > + }
>> >
>> > list_for_each_entry(m, &metric_list, nd) {
>> > if (m->evlist)
>> > --
>> > 2.34.0.rc0.344.g81b53c2807-goog
>>
>> --
>>
>> - Arnaldo
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-11-07 9:05 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-11-05 16:46 [PATCH] perf metric: Fix memory leaks Ian Rogers
2021-11-06 19:51 ` Arnaldo Carvalho de Melo
[not found] ` <CAP-5=fVMtO6zd_Qm9rJkMNGB==606hW-PRiQ7VCWFJLpJRmyBQ@mail.gmail.com>
2021-11-07 9:05 ` Ian Rogers
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).