From: Ian Rogers <irogers@google.com>
To: James Clark <james.clark@arm.com>
Cc: linux-perf-users@vger.kernel.org, atrajeev@linux.vnet.ibm.com,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Namhyung Kim <namhyung@kernel.org>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
"Liang, Kan" <kan.liang@linux.intel.com>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] perf symbols: Fix ownership of string in dso__load_vmlinux()
Date: Wed, 8 May 2024 15:14:06 -0700 [thread overview]
Message-ID: <CAP-5=fWJCfz6VPRC+A4pcsGYVBTv_ANis6kxByELVRiOWsPhxQ@mail.gmail.com> (raw)
In-Reply-To: <20240507141210.195939-5-james.clark@arm.com>
On Tue, May 7, 2024 at 7:13 AM James Clark <james.clark@arm.com> wrote:
>
> The linked commit updated dso__load_vmlinux() to call
> dso__set_long_name() before loading the symbols. Loading the symbols may
> not succeed but dso__set_long_name() takes ownership of the string. The
> two callers of this function free the string themselves on failure
> cases, resulting in the following error:
>
> $ perf record -- ls
> $ perf report
>
> free(): double free detected in tcache 2
>
> Fix it by always taking ownership of the string, even on failure. This
> means the string is either freed at the very first early exit condition,
> or later when the dso is deleted or the long name is replaced. Now no
> special return value is needed to signify that the caller needs to
> free the string.
>
> Fixes: e59fea47f83e ("perf symbols: Fix DSO kernel load and symbol process to correctly map DSO to its long_name, type and adjust_symbols")
> Signed-off-by: James Clark <james.clark@arm.com>
Reviewed-by: Ian Rogers <irogers@google.com>
Thanks,
Ian
> ---
> tools/perf/util/symbol.c | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index e98dfe766da3..6a0900dcdfd3 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1977,6 +1977,10 @@ int dso__load(struct dso *dso, struct map *map)
> return ret;
> }
>
> +/*
> + * Always takes ownership of vmlinux when vmlinux_allocated == true, even if
> + * it returns an error.
> + */
> int dso__load_vmlinux(struct dso *dso, struct map *map,
> const char *vmlinux, bool vmlinux_allocated)
> {
> @@ -1995,8 +1999,11 @@ int dso__load_vmlinux(struct dso *dso, struct map *map,
> else
> symtab_type = DSO_BINARY_TYPE__VMLINUX;
>
> - if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type))
> + if (symsrc__init(&ss, dso, symfs_vmlinux, symtab_type)) {
> + if (vmlinux_allocated)
> + free((char *) vmlinux);
> return -1;
> + }
>
> /*
> * dso__load_sym() may copy 'dso' which will result in the copies having
> @@ -2039,7 +2046,6 @@ int dso__load_vmlinux_path(struct dso *dso, struct map *map)
> err = dso__load_vmlinux(dso, map, filename, true);
> if (err > 0)
> goto out;
> - free(filename);
> }
> out:
> return err;
> @@ -2191,7 +2197,6 @@ static int dso__load_kernel_sym(struct dso *dso, struct map *map)
> err = dso__load_vmlinux(dso, map, filename, true);
> if (err > 0)
> return err;
> - free(filename);
> }
>
> if (!symbol_conf.ignore_vmlinux && vmlinux_path != NULL) {
> --
> 2.34.1
>
next prev parent reply other threads:[~2024-05-08 22:14 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-05-07 14:12 [PATCH 0/4] perf maps/symbols: Various assert fixes James Clark
2024-05-07 14:12 ` [PATCH 1/4] perf symbols: Remove map from list before updating addresses James Clark
2024-05-07 14:12 ` [PATCH 2/4] perf maps: Re-use __maps__free_maps_by_name() James Clark
2024-05-08 4:06 ` Namhyung Kim
2024-05-08 22:06 ` Ian Rogers
2024-05-07 14:12 ` [PATCH 3/4] perf symbols: Update kcore map before merging in remaining symbols James Clark
2024-05-08 4:10 ` Namhyung Kim
2024-05-08 9:14 ` James Clark
2024-05-08 14:19 ` Leo Yan
2024-05-07 14:12 ` [PATCH 4/4] perf symbols: Fix ownership of string in dso__load_vmlinux() James Clark
2024-05-08 22:14 ` Ian Rogers [this message]
2024-05-07 15:11 ` [PATCH 0/4] perf maps/symbols: Various assert fixes Arnaldo Carvalho de Melo
2024-05-07 15:12 ` Arnaldo Carvalho de Melo
2024-05-08 7:52 ` James Clark
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAP-5=fWJCfz6VPRC+A4pcsGYVBTv_ANis6kxByELVRiOWsPhxQ@mail.gmail.com' \
--to=irogers@google.com \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=atrajeev@linux.vnet.ibm.com \
--cc=james.clark@arm.com \
--cc=jolsa@kernel.org \
--cc=kan.liang@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).