Re: [PATCH] perf report: Fix a memory leak for perf_env on AMD

[Namhyung Kim]

On Mon, Mar 10, 2025 at 05:49:48PM -0700, Ian Rogers wrote:
> On Mon, Mar 10, 2025 at 5:04 PM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
> >
> > The env.pmu_mapping can be leaked when it reads data from a pipe on AMD.
> > For a pipe data, it reads the header data including pmu_mapping from
> > PERF_RECORD_HEADER_FEATURE runtime.  But it's already set in:
> >
> >   perf_session__new()
> >     __perf_session__new()
> >       evlist__init_trace_event_sample_raw()
> >         evlist__has_amd_ibs()
> >           perf_env__nr_pmu_mappings()
> >
> > Then it'll overwrite that when it processes the HEADER_FEATURE record.
> > Here's a report from address sanitizer.
> >
> >   Direct leak of 2689 byte(s) in 1 object(s) allocated from:
> >     #0 0x7fed8f814596 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
> >     #1 0x5595a7d416b1 in strbuf_grow util/strbuf.c:64
> >     #2 0x5595a7d414ef in strbuf_init util/strbuf.c:25
> >     #3 0x5595a7d0f4b7 in perf_env__read_pmu_mappings util/env.c:362
> >     #4 0x5595a7d12ab7 in perf_env__nr_pmu_mappings util/env.c:517
> >     #5 0x5595a7d89d2f in evlist__has_amd_ibs util/amd-sample-raw.c:315
> >     #6 0x5595a7d87fb2 in evlist__init_trace_event_sample_raw util/sample-raw.c:23
> >     #7 0x5595a7d7f893 in __perf_session__new util/session.c:179
> >     #8 0x5595a7b79572 in perf_session__new util/session.h:115
> >     #9 0x5595a7b7e9dc in cmd_report builtin-report.c:1603
> >     #10 0x5595a7c019eb in run_builtin perf.c:351
> >     #11 0x5595a7c01c92 in handle_internal_command perf.c:404
> >     #12 0x5595a7c01deb in run_argv perf.c:448
> >     #13 0x5595a7c02134 in main perf.c:556
> >     #14 0x7fed85833d67 in __libc_start_call_main ../sysdeps/nptl/libc_start_call_main.h:58
> >
> > Let's free the existing pmu_mapping data if any.
> 
> Can we simplify `evlist__has_amd_ibs` as there seems no reason to be
> creating/parsing strings. For example:
> ```
> bool evlist__has_amd_ibs(struct evlist *evlist)
> {
>    return perf_pmus__find("ibs_op") != NULL ||
> perf_pmus__find("ibs_fetch") != NULL;
> }
> ```

I think the idea is to find the pmu from the recorded data file not 
at runtime. 


> Similarly the variables `ibs_fetch_type` and `ibs_op_type` can be
> found with `perf_pmus__find("ibs_op")->type` and
> `perf_pmus__find("ibs_op")->type` respectively. It probably makes more
> sense to just cache access to the PMUs rather than do the finds all
> the time.
> 
> I wonder there is a problem with the pmus alternative that currently
> the pmus are global and not part of say the session, not loaded from
> the perf.data file, etc. So use on a different machine than the
> recording could be broken by what I suggest. I guess then, just
> throwing in the free is simplest.

Agreed.  It could be saved in a session or env.

For now, let's free the string to fix the leak quickly.

Thanks,
Namhyung

> 
> 
> > Cc: Ravi Bangoria <ravi.bangoria@xxxxxxx>
> > Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> > ---
> >  tools/perf/util/header.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> > index 1900965f87527948..e3cdc3b7b4ab2409 100644
> > --- a/tools/perf/util/header.c
> > +++ b/tools/perf/util/header.c
> > @@ -2770,6 +2770,8 @@ static int process_pmu_mappings(struct feat_fd *ff, void *data __maybe_unused)
> >                 free(name);
> >                 pmu_num--;
> >         }
> > +       /* AMD may set it by evlist__has_amd_ibs() from perf_session__new() */
> > +       free(ff->ph->env.pmu_mappings);
> >         ff->ph->env.pmu_mappings = strbuf_detach(&sb, NULL);
> >         return 0;
> >
> > --
> > 2.49.0.rc0.332.g42c0ae87b1-goog
> >