linux-kernel - Re: [PATCH] perf report: Fix a memory leak for perf

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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAP-5=fWSU8m1eae=5GFTpVTz1CPtwv=eDmivT1v14VjT40SptA@mail.gmail.com>
Date: Mon, 10 Mar 2025 17:49:48 -0700
From: Ian Rogers <irogers@...gle.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Kan Liang <kan.liang@...ux.intel.com>, 
	Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, 
	Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, 
	LKML <linux-kernel@...r.kernel.org>, linux-perf-users@...r.kernel.org, 
	Ravi Bangoria <ravi.bangoria@....com>
Subject: Re: [PATCH] perf report: Fix a memory leak for perf_env on AMD

On Mon, Mar 10, 2025 at 5:04 PM Namhyung Kim <namhyung@...nel.org> 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;
}
```

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.

Thanks,
Ian



> Cc: Ravi Bangoria <ravi.bangoria@....com>
> Signed-off-by: Namhyung Kim <namhyung@...nel.org>
> ---
>  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
>