Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230630153840.858668-1-james.clark@arm.com>
In-Reply-To: <20230630153840.858668-1-james.clark@arm.com>
From:   Ian Rogers <irogers@google.com>
Date:   Fri, 30 Jun 2023 08:49:28 -0700
Message-ID: <CAP-5=fWMu7g9DWbBatbACV0VmThKNLLg0=Uff-oH48XEsrpJ3g@mail.gmail.com>
Subject: Re: [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name()
To:     James Clark <james.clark@arm.com>
Cc:     linux-perf-users@vger.kernel.org,
        Peter Zijlstra <peterz@infradead.org>,
        Ingo Molnar <mingo@redhat.com>,
        Arnaldo Carvalho de Melo <acme@kernel.org>,
        Mark Rutland <mark.rutland@arm.com>,
        Alexander Shishkin <alexander.shishkin@linux.intel.com>,
        Jiri Olsa <jolsa@kernel.org>,
        Namhyung Kim <namhyung@kernel.org>,
        Adrian Hunter <adrian.hunter@intel.com>,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, Jun 30, 2023 at 8:39=E2=80=AFAM James Clark <james.clark@arm.com> w=
rote:
>
> found_idx and s aren't initialized, so if no symbol is found then the
> assert at the end will index off the end of the array causing a
> segfault. The function also doesn't return NULL when the symbol isn't
> found even if the assert passes. Fix it by initializing the values and
> only setting them when something is found.
>
> Fixes the following test failure:
>
>   $ perf test 1
>   1: vmlinux symtab matches kallsyms     : FAILED!
>
> Fixes: 259dce914e93 ("perf symbol: Remove symbol_name_rb_node")
> Signed-off-by: James Clark <james.clark@arm.com>

Thanks, and thanks for the Fixes.

Acked-by: Ian Rogers <irogers@google.com>

> ---
>  tools/perf/util/symbol.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index bc79291b9f3b..f849f9ef68e6 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -495,7 +495,10 @@ static struct symbol *symbols__find_by_name(struct s=
ymbol *symbols[],
>                                             size_t *found_idx)
>  {
>         size_t i, lower =3D 0, upper =3D symbols_len;
> -       struct symbol *s;
> +       struct symbol *s =3D NULL;
> +
> +       if (found_idx)
> +               *found_idx =3D SIZE_MAX;
>
>         if (!symbols_len)
>                 return NULL;
> @@ -504,8 +507,7 @@ static struct symbol *symbols__find_by_name(struct sy=
mbol *symbols[],
>                 int cmp;
>
>                 i =3D (lower + upper) / 2;
> -               s =3D symbols[i];
> -               cmp =3D symbol__match_symbol_name(s->name, name, includes=
);
> +               cmp =3D symbol__match_symbol_name(symbols[i]->name, name,=
 includes);
>
>                 if (cmp > 0)
>                         upper =3D i;
> @@ -514,10 +516,11 @@ static struct symbol *symbols__find_by_name(struct =
symbol *symbols[],
>                 else {
>                         if (found_idx)
>                                 *found_idx =3D i;
> +                       s =3D symbols[i];
>                         break;
>                 }
>         }
> -       if (includes !=3D SYMBOL_TAG_INCLUDE__DEFAULT_ONLY) {
> +       if (s && includes !=3D SYMBOL_TAG_INCLUDE__DEFAULT_ONLY) {
>                 /* return first symbol that has same name (if any) */
>                 for (; i > 0; i--) {
>                         struct symbol *tmp =3D symbols[i - 1];
> @@ -525,13 +528,12 @@ static struct symbol *symbols__find_by_name(struct =
symbol *symbols[],
>                         if (!arch__compare_symbol_names(tmp->name, s->nam=
e)) {
>                                 if (found_idx)
>                                         *found_idx =3D i - 1;
> +                               s =3D tmp;
>                         } else
>                                 break;
> -
> -                       s =3D tmp;
>                 }
>         }
> -       assert(!found_idx || s =3D=3D symbols[*found_idx]);
> +       assert(!found_idx || !s || s =3D=3D symbols[*found_idx]);
>         return s;
>  }
>
> --
> 2.34.1
>