Re: [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name()

[Ian Rogers]

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

Thanks, and thanks for the Fixes.

Acked-by: Ian Rogers <irogers@xxxxxxxxxx>

> ---
>  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
>