linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name()
@ 2023-06-30 15:38 James Clark
  2023-06-30 15:49 ` Ian Rogers
  0 siblings, 1 reply; 3+ messages in thread
From: James Clark @ 2023-06-30 15:38 UTC (permalink / raw)
  To: linux-perf-users, irogers
  Cc: James Clark, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Adrian Hunter, linux-kernel

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


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name()
  2023-06-30 15:38 [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name() James Clark
@ 2023-06-30 15:49 ` Ian Rogers
  2023-07-02  1:02   ` Namhyung Kim
  0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2023-06-30 15:49 UTC (permalink / raw)
  To: James Clark
  Cc: linux-perf-users, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Namhyung Kim, Adrian Hunter, linux-kernel

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

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name()
  2023-06-30 15:49 ` Ian Rogers
@ 2023-07-02  1:02   ` Namhyung Kim
  0 siblings, 0 replies; 3+ messages in thread
From: Namhyung Kim @ 2023-07-02  1:02 UTC (permalink / raw)
  To: Ian Rogers
  Cc: James Clark, linux-perf-users, Peter Zijlstra, Ingo Molnar,
	Arnaldo Carvalho de Melo, Mark Rutland, Alexander Shishkin,
	Jiri Olsa, Adrian Hunter, linux-kernel

On Fri, Jun 30, 2023 at 8:49 AM Ian Rogers <irogers@google.com> wrote:
>
> On Fri, Jun 30, 2023 at 8:39 AM James Clark <james.clark@arm.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@arm.com>
>
> Thanks, and thanks for the Fixes.
>
> Acked-by: Ian Rogers <irogers@google.com>

Applied to perf-tools-next, thanks!

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-07-02  1:02 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-06-30 15:38 [PATCH] perf symbol: Fix uninitialized return value in symbols__find_by_name() James Clark
2023-06-30 15:49 ` Ian Rogers
2023-07-02  1:02   ` Namhyung Kim

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 NNTP newsgroup(s).