linux-kernel - Re: [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by

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=fWMu7g9DWbBatbACV0VmThKNLLg0=Uff-oH48XEsrpJ3g@mail.gmail.com>
Date:   Fri, 30 Jun 2023 08:49:28 -0700
From:   Ian Rogers <irogers@...gle.com>
To:     James Clark <james.clark@....com>
Cc:     linux-perf-users@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Jiri Olsa <jolsa@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Adrian Hunter <adrian.hunter@...el.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name()

On Fri, Jun 30, 2023 at 8:39 AM James Clark <james.clark@....com> wrote:
>
> 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@....com>

Thanks, and thanks for the Fixes.

Acked-by: Ian Rogers <irogers@...gle.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 symbol *symbols[],
>                                             size_t *found_idx)
>  {
>         size_t i, lower = 0, upper = symbols_len;
> -       struct symbol *s;
> +       struct symbol *s = NULL;
> +
> +       if (found_idx)
> +               *found_idx = SIZE_MAX;
>
>         if (!symbols_len)
>                 return NULL;
> @@ -504,8 +507,7 @@ static struct symbol *symbols__find_by_name(struct symbol *symbols[],
>                 int cmp;
>
>                 i = (lower + upper) / 2;
> -               s = symbols[i];
> -               cmp = symbol__match_symbol_name(s->name, name, includes);
> +               cmp = symbol__match_symbol_name(symbols[i]->name, name, includes);
>
>                 if (cmp > 0)
>                         upper = i;
> @@ -514,10 +516,11 @@ static struct symbol *symbols__find_by_name(struct symbol *symbols[],
>                 else {
>                         if (found_idx)
>                                 *found_idx = i;
> +                       s = symbols[i];
>                         break;
>                 }
>         }
> -       if (includes != SYMBOL_TAG_INCLUDE__DEFAULT_ONLY) {
> +       if (s && includes != SYMBOL_TAG_INCLUDE__DEFAULT_ONLY) {
>                 /* return first symbol that has same name (if any) */
>                 for (; i > 0; i--) {
>                         struct symbol *tmp = 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->name)) {
>                                 if (found_idx)
>                                         *found_idx = i - 1;
> +                               s = tmp;
>                         } else
>                                 break;
> -
> -                       s = tmp;
>                 }
>         }
> -       assert(!found_idx || s == symbols[*found_idx]);
> +       assert(!found_idx || !s || s == symbols[*found_idx]);
>         return s;
>  }
>
> --
> 2.34.1
>