linux-kernel - Re: [PATCH] perf metric: Fix memory leaks.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fU7vh4XAe-jEa13tzy2x+a3oF+tfJfy2_Oxs5q1SSNYEw@mail.gmail.com>
Date:   Sun, 7 Nov 2021 01:05:30 -0800
From:   Ian Rogers <irogers@...gle.com>
To:     Arnaldo Carvalho de Melo <acme@...nel.org>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...hat.com>,
        Namhyung Kim <namhyung@...nel.org>,
        John Garry <john.garry@...wei.com>,
        linux-perf-users <linux-perf-users@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH] perf metric: Fix memory leaks.

On Sat, Nov 6, 2021 at 5:36 PM Ian Rogers <irogers@...gle.com> wrote:
>
>
>
> On Sat, Nov 6, 2021, 12:51 PM Arnaldo Carvalho de Melo <acme@...nel.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@...gle.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