Received-SPF: pass (google.com: domain of linux-kernel+bounces-173886-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240507141210.195939-1-james.clark@arm.com> <20240507141210.195939-5-james.clark@arm.com>
In-Reply-To: <20240507141210.195939-5-james.clark@arm.com>
From: Ian Rogers <irogers@google.com>
Date: Wed, 8 May 2024 15:14:06 -0700
Message-ID: <CAP-5=fWJCfz6VPRC+A4pcsGYVBTv_ANis6kxByELVRiOWsPhxQ@mail.gmail.com>
Subject: Re: [PATCH 4/4] perf symbols: Fix ownership of string in dso__load_vmlinux()
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
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, May 7, 2024 at 7:13=E2=80=AFAM James Clark <james.clark@arm.com> wr=
ote:
>
> 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 proces=
s 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 =3D=3D 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 =3D 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 =3D 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, st=
ruct map *map)
>                 err =3D dso__load_vmlinux(dso, map, filename, true);
>                 if (err > 0)
>                         return err;
> -               free(filename);
>         }
>
>         if (!symbol_conf.ignore_vmlinux && vmlinux_path !=3D NULL) {
> --
> 2.34.1
>