Re: [PATCH] perf metric: Fix memory leaks.

[Ian Rogers <irogers@google.com>]

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