[PATCH v3 1/4] perf symbol: Fix symbol loading from libbfd

[PATCH v3 1/4] perf symbol: Fix symbol loading from libbfd

[Namhyung Kim]

The dso__load() should return a positive number (number of symbols
loaded) when it succeded to load symbols.  But dso__load_bfd_symbols()
only returns 0 and it's hard to check if it actually loaded symbols or
not.  Let's return the value of dso__has_symbol() and check it in
dso__load() so that it can return 1 for success.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/pe-file-parsing.c | 2 +-
 tools/perf/util/libbfd.c           | 2 +-
 tools/perf/util/symbol.c           | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 30c7da79e109b480..3979f38bf127c218 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -59,7 +59,7 @@ static int run_dir(const char *d)
 	TEST_ASSERT_VAL("Failed to get dso", dso);
 
 	ret = dso__load_bfd_symbols(dso, debugfile);
-	TEST_ASSERT_VAL("Failed to load symbols", ret == 0);
+	TEST_ASSERT_VAL("Failed to load symbols", ret > 0);
 
 	dso__sort_by_name(dso);
 	sym = dso__find_symbol_by_name(dso, "main", &idx);
diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
index a907d7c1ac948808..a4dcb406c79e9cee 100644
--- a/tools/perf/util/libbfd.c
+++ b/tools/perf/util/libbfd.c
@@ -463,7 +463,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
 	symbols__fixup_duplicate(dso__symbols(dso));
 	dso__set_adjust_symbols(dso, true);
 
-	err = 0;
+	err = dso__has_symbols(dso);
 out_free:
 	free(symbols);
 out_close:
diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 116870595f75c7dd..2b1c1b8d9652e720 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1921,8 +1921,8 @@ int dso__load(struct dso *dso, struct map *map)
 		if (nsexit)
 			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 
-		if (bfdrc == 0) {
-			ret = 0;
+		if (bfdrc > 0) {
+			ret = 1;
 			break;
 		}
 
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v3 2/4] iperf symbol: Add missing symsrc__destroy() for libbfd

[Namhyung Kim]

When libbfd loaded symbols and there's a pending syms_ss, it'd be lost
or overwrite existing symbols.  Let's destroy it before breaking the
loop so that it can safely return from the function.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
Reviewed-by: Ian Rogers <irogers@google.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 2b1c1b8d9652e720..02845b69ae6b5fcf 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1923,7 +1923,7 @@ int dso__load(struct dso *dso, struct map *map)
 
 		if (bfdrc > 0) {
 			ret = 1;
-			break;
+			goto out_destroy;
 		}
 
 		if (!is_reg || sirc < 0)
@@ -1975,6 +1975,7 @@ int dso__load(struct dso *dso, struct map *map)
 			ret += nr_plt;
 	}
 
+out_destroy:
 	for (; ss_pos > 0; ss_pos--)
 		symsrc__destroy(&ss_[ss_pos - 1]);
 out_free:
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v3 3/4] perf symbol: Set dso symtab type for libbfd

[Namhyung Kim]

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
index 02845b69ae6b5fcf..b61a574ce9a30721 100644
--- a/tools/perf/util/symbol.c
+++ b/tools/perf/util/symbol.c
@@ -1922,6 +1922,7 @@ int dso__load(struct dso *dso, struct map *map)
 			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
 
 		if (bfdrc > 0) {
+			dso__set_symtab_type(dso, symtab_type);
 			ret = 1;
 			goto out_destroy;
 		}
-- 
2.56.0.385.gd3acb90ef8-goog

[PATCH v3 4/4] perf test: Update PE file test for other locations

[Namhyung Kim]

The test__pe_file_parsing() expects the test is in the tools/perf
directory, but it could be root or a separate build directory.  Let's
update the logic to try other paths like in the shell_tests__dir_fd().

Also add some debug messages to inform users why it's skipped.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/pe-file-parsing.c | 20 ++++++++++++--------
 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 3979f38bf127c218..190520ffc3ad6d0c 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -74,17 +74,20 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 {
 	struct stat st;
 	char path_dir[PATH_MAX];
-
-	/* First try development tree tests. */
-	if (!lstat("./tests", &st))
-		return run_dir("./tests");
-
-	/* Then installed path. */
+	const char * const tests_dirs[] = {
+		"./tools/perf/tests",
+		"./tests",
+		"./source/tests",
+		path_dir,
+	};
 	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
 
-	if (!lstat(path_dir, &st))
-		return run_dir(path_dir);
+	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
+		if (!lstat(tests_dirs[i], &st))
+			return run_dir(tests_dirs[i]);
+	}
 
+	pr_debug("cannot find the tests directory\n");
 	return TEST_SKIP;
 }
 
@@ -93,6 +96,7 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 			  int subtest __maybe_unused)
 {
+	pr_debug("libbfd is not available\n");
 	return TEST_SKIP;
 }
 
-- 
2.56.0.385.gd3acb90ef8-goog

Re: [PATCH v3 2/4] iperf symbol: Add missing symsrc__destroy() for libbfd

[sashiko-bot]

> When libbfd loaded symbols and there's a pending syms_ss, it'd be lost
> or overwrite existing symbols.  Let's destroy it before breaking the
> loop so that it can safely return from the function.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
> Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012235938.87912-1-namhyung@kernel.org?part=2

Re: [PATCH v3 3/4] perf symbol: Set dso symtab type for libbfd

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf: missing dso__set_binary_type() initialization for libbfd bypass
--

commit 3d059164c2c17ae3b0ec344af1e8527af000eee5
Author: Namhyung Kim <namhyung@kernel.org>

perf symbol: Set dso symtab type for libbfd

It missed to set the dso symtab type when the symbols are loaded by
libbfd.  When it happens both syms_ss and runtime_ss may not be set
and won't call dso__load_sym().

> diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> index 4878d4292eb85..a90ce714b174b 100644
> --- a/tools/perf/util/symbol.c
> +++ b/tools/perf/util/symbol.c
> @@ -1925,6 +1925,7 @@ int dso__load(struct dso *dso, struct map *map)
>  			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
>  
>  		if (bfdrc > 0) {
> +			dso__set_symtab_type(dso, symtab_type);

[Severity: Medium]
When dso__load_bfd_symbols() succeeds and we take this early return path, does
this code also need to set the DSO's binary_type and cross-platform
architecture flags?

By taking this goto out_destroy, the rest of the DSO initialization loop is
bypassed. This leaves the binary_type as DSO_BINARY_TYPE__NOT_FOUND. Later,
when dso__data_fd() attempts to read instruction bytes for perf annotate or
Intel PT decoding, try_to_open_dso() will restrict its search to only
BUILD_ID_CACHE and SYSTEM_PATH_DSO since the actual discovery path wasn't
recorded.

Additionally, bypassing dso__load_sym() means is_64_bit won't be set, which
causes it to incorrectly default to the host perf binary's pointer size. This
breaks cross-platform analysis of 32-bit files on 64-bit hosts.

>  			ret = 1;
>  			goto out_destroy;
>  		}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012235938.87912-1-namhyung@kernel.org?part=3

Re: [PATCH v3 1/4] perf symbol: Fix symbol loading from libbfd

[sashiko-bot]

> The dso__load() should return a positive number (number of symbols
> loaded) when it succeded to load symbols.  But dso__load_bfd_symbols()
> only returns 0 and it's hard to check if it actually loaded symbols or
> not.  Let's return the value of dso__has_symbol() and check it in
> dso__load() so that it can return 1 for success.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-162540@kernel.org
> Fixes: eac9a4342e5447cad ("perf symbols: Try reading the symbol table with libbfd")
> Reviewed-by: Ian Rogers <irogers@google.com>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012235938.87912-1-namhyung@kernel.org?part=1

Re: [PATCH v3 4/4] perf test: Update PE file test for other locations

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] perf test: unconditional leak of get_argv_exec_path() and NULL deref
--

commit 9107444f4267d3db60d0e4c357f3b28957bdb1bc
Author: Namhyung Kim <namhyung@kernel.org>

perf test: Update PE file test for other locations

This commit updates the PE file parsing test to check multiple directory paths
for the test data. The logic is modified to try other paths similar to the
shell_tests__dir_fd() fallback mechanism.

> diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> index 3979f38bf127c..190520ffc3ad6 100644
> --- a/tools/perf/tests/pe-file-parsing.c
> +++ b/tools/perf/tests/pe-file-parsing.c
> @@ -74,17 +74,20 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
>  {
>  	struct stat st;
>  	char path_dir[PATH_MAX];
> -
> -	/* First try development tree tests. */
> -	if (!lstat("./tests", &st))
> -		return run_dir("./tests");
> -
> -	/* Then installed path. */
> +	const char * const tests_dirs[] = {
> +		"./tools/perf/tests",
> +		"./tests",
> +		"./source/tests",
> +		path_dir,
> +	};
>  	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());

[Severity: Low]
Can this code in test__pe_file_parsing() lead to a memory leak and a potential
NULL pointer dereference?

The get_argv_exec_path() function returns a dynamically allocated string that
must be freed by the caller. Passing it directly to snprintf() inline causes
the pointer to be permanently lost.

Additionally, since the patch moved this snprintf() call before the directory
search loop, it now unconditionally allocates this string even if an earlier
directory in the tests_dirs array would have succeeded.

Furthermore, if get_argv_exec_path() fails under memory pressure and returns
NULL, snprintf("%s") could crash on strictly compliant libcs like musl.

>  
> -	if (!lstat(path_dir, &st))
> -		return run_dir(path_dir);
> +	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> +		if (!lstat(tests_dirs[i], &st))
> +			return run_dir(tests_dirs[i]);
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012235938.87912-1-namhyung@kernel.org?part=4

Re: [PATCH v3 3/4] perf symbol: Set dso symtab type for libbfd

[Namhyung Kim]

On Tue, Oct 13, 2026 at 12:08:01AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Medium] perf: missing dso__set_binary_type() initialization for libbfd bypass
> --
> 
> commit 3d059164c2c17ae3b0ec344af1e8527af000eee5
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf symbol: Set dso symtab type for libbfd
> 
> It missed to set the dso symtab type when the symbols are loaded by
> libbfd.  When it happens both syms_ss and runtime_ss may not be set
> and won't call dso__load_sym().
> 
> > diff --git a/tools/perf/util/symbol.c b/tools/perf/util/symbol.c
> > index 4878d4292eb85..a90ce714b174b 100644
> > --- a/tools/perf/util/symbol.c
> > +++ b/tools/perf/util/symbol.c
> > @@ -1925,6 +1925,7 @@ int dso__load(struct dso *dso, struct map *map)
> >  			nsinfo__mountns_enter(dso__nsinfo(dso), &nsc);
> >  
> >  		if (bfdrc > 0) {
> > +			dso__set_symtab_type(dso, symtab_type);
> 
> [Severity: Medium]
> When dso__load_bfd_symbols() succeeds and we take this early return path, does
> this code also need to set the DSO's binary_type and cross-platform
> architecture flags?
> 
> By taking this goto out_destroy, the rest of the DSO initialization loop is
> bypassed. This leaves the binary_type as DSO_BINARY_TYPE__NOT_FOUND. Later,
> when dso__data_fd() attempts to read instruction bytes for perf annotate or
> Intel PT decoding, try_to_open_dso() will restrict its search to only
> BUILD_ID_CACHE and SYSTEM_PATH_DSO since the actual discovery path wasn't
> recorded.

As I said before, it should be handled in a separate patch series.

> 
> Additionally, bypassing dso__load_sym() means is_64_bit won't be set, which
> causes it to incorrectly default to the host perf binary's pointer size. This
> breaks cross-platform analysis of 32-bit files on 64-bit hosts.

Looks like a pre-existing issue.  I'll add it in a separate patch.

Thanks,
Namhyung

> 
> >  			ret = 1;
> >  			goto out_destroy;
> >  		}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261012235938.87912-1-namhyung@kernel.org?part=3

Re: [PATCH v3 4/4] perf test: Update PE file test for other locations

[Namhyung Kim]

On Tue, Oct 13, 2026 at 12:08:54AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] perf test: unconditional leak of get_argv_exec_path() and NULL deref
> --
> 
> commit 9107444f4267d3db60d0e4c357f3b28957bdb1bc
> Author: Namhyung Kim <namhyung@kernel.org>
> 
> perf test: Update PE file test for other locations
> 
> This commit updates the PE file parsing test to check multiple directory paths
> for the test data. The logic is modified to try other paths similar to the
> shell_tests__dir_fd() fallback mechanism.
> 
> > diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
> > index 3979f38bf127c..190520ffc3ad6 100644
> > --- a/tools/perf/tests/pe-file-parsing.c
> > +++ b/tools/perf/tests/pe-file-parsing.c
> > @@ -74,17 +74,20 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
> >  {
> >  	struct stat st;
> >  	char path_dir[PATH_MAX];
> > -
> > -	/* First try development tree tests. */
> > -	if (!lstat("./tests", &st))
> > -		return run_dir("./tests");
> > -
> > -	/* Then installed path. */
> > +	const char * const tests_dirs[] = {
> > +		"./tools/perf/tests",
> > +		"./tests",
> > +		"./source/tests",
> > +		path_dir,
> > +	};
> >  	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
> 
> [Severity: Low]
> Can this code in test__pe_file_parsing() lead to a memory leak and a potential
> NULL pointer dereference?
> 
> The get_argv_exec_path() function returns a dynamically allocated string that
> must be freed by the caller. Passing it directly to snprintf() inline causes
> the pointer to be permanently lost.
> 
> Additionally, since the patch moved this snprintf() call before the directory
> search loop, it now unconditionally allocates this string even if an earlier
> directory in the tests_dirs array would have succeeded.
> 
> Furthermore, if get_argv_exec_path() fails under memory pressure and returns
> NULL, snprintf("%s") could crash on strictly compliant libcs like musl.

These are pre-existing issues, will fix separately.

Thanks,
Namhyung

> 
> >  
> > -	if (!lstat(path_dir, &st))
> > -		return run_dir(path_dir);
> > +	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
> > +		if (!lstat(tests_dirs[i], &st))
> > +			return run_dir(tests_dirs[i]);
> > +	}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20261012235938.87912-1-namhyung@kernel.org?part=4