lkml.org 
[lkml]   [2021]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH] perf metric: Fix memory leaks.
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

\
 
 \ /
  Last update: 2021-11-07 10:07    [W:0.045 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site