[PATCH v1 1/5] perf test: Fix a memory leak in PE file parsing test

[PATCH v1 1/5] perf test: Fix a memory leak in PE file parsing test

[Namhyung Kim]

The get_argv_exec_path() returns a dynamically allocated string so it
should be freed.  Let's check the return value first so that it
doesn't crash snprintf().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-168781@kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/pe-file-parsing.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/tools/perf/tests/pe-file-parsing.c b/tools/perf/tests/pe-file-parsing.c
index 190520ffc3ad6d0c..ed95aa5d8ca2af92 100644
--- a/tools/perf/tests/pe-file-parsing.c
+++ b/tools/perf/tests/pe-file-parsing.c
@@ -74,19 +74,26 @@ static int test__pe_file_parsing(struct test_suite *test __maybe_unused,
 {
 	struct stat st;
 	char path_dir[PATH_MAX];
-	const char * const tests_dirs[] = {
+	static const char * const tests_dirs[] = {
 		"./tools/perf/tests",
 		"./tests",
 		"./source/tests",
-		path_dir,
 	};
-	snprintf(path_dir, PATH_MAX, "%s/tests", get_argv_exec_path());
+	char *exec_path;
 
 	for (size_t i = 0; i < ARRAY_SIZE(tests_dirs); ++i) {
 		if (!lstat(tests_dirs[i], &st))
 			return run_dir(tests_dirs[i]);
 	}
 
+	exec_path = get_argv_exec_path();
+	TEST_ASSERT_VAL("Failed to allocate exec_path", exec_path);
+	snprintf(path_dir, PATH_MAX, "%s/tests", exec_path);
+	free(exec_path);
+
+	if (!lstat(path_dir, &st))
+		return run_dir(path_dir);
+
 	pr_debug("cannot find the tests directory\n");
 	return TEST_SKIP;
 }
-- 
2.56.0.176.g3575e65682-goog

[PATCH v1 2/5] perf test: Make sure maps to have a machine

[Namhyung Kim]

It's be easier if we could guarantee that maps always have a machine.
IIUC this is true for all real use cases, but only the test code has
an exception.  Let's get rid of it and make sure they have machines.

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/tests/maps.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/tools/perf/tests/maps.c b/tools/perf/tests/maps.c
index 2c05d62f40dcdf72..f40b68f5fa5fe2c1 100644
--- a/tools/perf/tests/maps.c
+++ b/tools/perf/tests/maps.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 #include <inttypes.h>
+#include <unistd.h>
 #include <linux/compiler.h>
 #include <linux/kernel.h>
 #include "tests.h"
@@ -100,8 +101,12 @@ static int test__maps__merge_in(struct test_suite *t __maybe_unused, int subtest
 	};
 	struct map *map_kcore1, *map_kcore2, *map_kcore3;
 	int ret;
-	struct maps *maps = maps__new(NULL);
+	struct machine *machine = machine__new_live(/*host_env=*/NULL,
+						    /*kernel_maps=*/false,
+						    getpid());
+	struct maps *maps = maps__new(machine);
 
+	TEST_ASSERT_VAL("failed to create machine", machine);
 	TEST_ASSERT_VAL("failed to create maps", maps);
 
 	for (i = 0; i < ARRAY_SIZE(bpf_progs); i++) {
@@ -159,6 +164,7 @@ static int test__maps__merge_in(struct test_suite *t __maybe_unused, int subtest
 	map__zput(map_kcore1);
 	map__zput(map_kcore2);
 	map__zput(map_kcore3);
+	machine__delete(machine);
 	return TEST_OK;
 }
 
@@ -189,8 +195,12 @@ static int test__maps__fixup_overlap_and_insert(struct test_suite *t __maybe_unu
 	struct map *map_split, *map_eclipse;
 	int ret;
 	unsigned int i;
-	struct maps *maps = maps__new(NULL);
+	struct machine *machine = machine__new_live(/*host_env=*/NULL,
+						    /*kernel_maps=*/false,
+						    getpid());
+	struct maps *maps = maps__new(machine);
 
+	TEST_ASSERT_VAL("failed to create machine", machine);
 	TEST_ASSERT_VAL("failed to create maps", maps);
 
 	for (i = 0; i < ARRAY_SIZE(initial_maps); i++) {
@@ -230,6 +240,7 @@ static int test__maps__fixup_overlap_and_insert(struct test_suite *t __maybe_unu
 	TEST_ASSERT_VAL("eclipse check failed", !ret);
 
 	maps__zput(maps);
+	machine__delete(machine);
 	return TEST_OK;
 }
 
-- 
2.56.0.176.g3575e65682-goog

[PATCH v1 3/5] perf tools: Remove NULL check for maps__machine()

[Namhyung Kim]

Now maps should have a machine, let's get rid of checks for the return
value.  There are already many places to assume it returns non-NULL,
so having this kind of check could give confusion that it can return
NULL (which is not true anymore).

Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/dlfilter.c | 3 +--
 tools/perf/util/event.c    | 3 ---
 tools/perf/util/map.c      | 9 +++------
 tools/perf/util/python.c   | 2 +-
 4 files changed, 5 insertions(+), 12 deletions(-)

diff --git a/tools/perf/util/dlfilter.c b/tools/perf/util/dlfilter.c
index e11e144af62bb979..7e179d1d84d78358 100644
--- a/tools/perf/util/dlfilter.c
+++ b/tools/perf/util/dlfilter.c
@@ -233,8 +233,7 @@ static const __u8 *dlfilter__insn(void *ctx, __u32 *len)
 		if (thread__maps(al->thread)) {
 			struct machine *machine = maps__machine(thread__maps(al->thread));
 
-			if (machine)
-				perf_sample__fetch_insn(d->sample, al->thread, machine);
+			perf_sample__fetch_insn(d->sample, al->thread, machine);
 		}
 	}
 
diff --git a/tools/perf/util/event.c b/tools/perf/util/event.c
index ea75816d126a14be..2f80e364659fea71 100644
--- a/tools/perf/util/event.c
+++ b/tools/perf/util/event.c
@@ -706,9 +706,6 @@ struct map *thread__find_map(struct thread *thread, u8 cpumode, u64 addr,
 	al->cpumode = cpumode;
 	al->filtered = 0;
 
-	if (machine == NULL)
-		return NULL;
-
 	if (cpumode == PERF_RECORD_MISC_KERNEL && perf_host) {
 		al->level = 'k';
 		maps = machine__kernel_maps(machine);
diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
index 41cdddc987eee710..d5319ffa88a7477c 100644
--- a/tools/perf/util/map.c
+++ b/tools/perf/util/map.c
@@ -538,13 +538,10 @@ u64 map__rip_2objdump(const struct map *map, u64 rip)
 	 */
 	if (kmap && is_entry_trampoline(kmap->name) && kmap->kmaps) {
 		struct machine *machine = maps__machine(kmap->kmaps);
+		struct map *kernel_map = machine__kernel_map(machine);
 
-		if (machine) {
-			struct map *kernel_map = machine__kernel_map(machine);
-
-			if (kernel_map)
-				map = kernel_map;
-		}
+		if (kernel_map)
+			map = kernel_map;
 	}
 
 	if (!dso__adjust_symbols(dso))
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 60dbe2dad5eb8073..42c587cd54a527a4 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -4536,7 +4536,7 @@ static PyObject *pyrf_call_return__new(struct call_return *cr)
 	if (cr->thread) {
 		struct maps *maps = thread__maps(cr->thread);
 
-		pevent->machine_pid = maps && maps__machine(maps) ? maps__machine(maps)->pid : 0;
+		pevent->machine_pid = maps ? maps__machine(maps)->pid : 0;
 		pevent->pid = thread__pid(cr->thread);
 		pevent->tid = thread__tid(cr->thread);
 	} else {
-- 
2.56.0.176.g3575e65682-goog

[PATCH v1 4/5] perf symbol: Check result when reading section headers

[Namhyung Kim]

Otherwise it may access uninitialized section header struct.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-170233@kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/symbol-elf.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/symbol-elf.c b/tools/perf/util/symbol-elf.c
index 1e2716c6af45712a..00ea0e19020fb256 100644
--- a/tools/perf/util/symbol-elf.c
+++ b/tools/perf/util/symbol-elf.c
@@ -196,13 +196,15 @@ Elf_Scn *elf_section_by_name(Elf *elf, GElf_Ehdr *ep,
 	while ((sec = elf_nextscn(elf, sec)) != NULL) {
 		char *str;
 
-		gelf_getshdr(sec, shp);
+		if (!gelf_getshdr(sec, shp))
+			goto next;
 		str = elf_strptr(elf, ep->e_shstrndx, shp->sh_name);
 		if (str && !strcmp(name, str)) {
 			if (idx)
 				*idx = cnt;
 			return sec;
 		}
+next:
 		++cnt;
 	}
 
-- 
2.56.0.176.g3575e65682-goog

[PATCH v1 5/5] perf symbol: Ensure dso__set_is_64_bit() for libbfd

[Namhyung Kim]

Otherwise it can confuse cross-platform analysis.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/r/sashiko-outbox-168776@kernel.org
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/util/libbfd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
index a4dcb406c79e9cee..9cc6216ce3adbcd0 100644
--- a/tools/perf/util/libbfd.c
+++ b/tools/perf/util/libbfd.c
@@ -462,6 +462,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
 	symbols__fixup_end(dso__symbols(dso), false);
 	symbols__fixup_duplicate(dso__symbols(dso));
 	dso__set_adjust_symbols(dso, true);
+	dso__set_is_64_bit(dso, bfd_get_arch_size(abfd) == 64);
 
 	err = dso__has_symbols(dso);
 out_free:
-- 
2.56.0.176.g3575e65682-goog

Re: [PATCH v1 4/5] perf symbol: Check result when reading section headers

[sashiko-bot]

> Otherwise it may access uninitialized section header struct.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-170233@kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH v1 1/5] perf test: Fix a memory leak in PE file parsing test

[sashiko-bot]

> The get_argv_exec_path() returns a dynamically allocated string so it
> should be freed.  Let's check the return value first so that it
> doesn't crash snprintf().
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/r/sashiko-outbox-168781@kernel.org
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH v1 3/5] perf tools: Remove NULL check for maps__machine()

[sashiko-bot]

> Now maps should have a machine, let's get rid of checks for the return
> value.  There are already many places to assume it returns non-NULL,
> so having this kind of check could give confusion that it can return
> NULL (which is not true anymore).
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH v1 2/5] perf test: Make sure maps to have a machine

[sashiko-bot]

> It's be easier if we could guarantee that maps always have a machine.
> IIUC this is true for all real use cases, but only the test code has
> an exception.  Let's get rid of it and make sure they have machines.
> 
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Sashiko has reviewed this patch and found no issues.

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

Re: [PATCH v1 5/5] perf symbol: Ensure dso__set_is_64_bit() for libbfd

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf symbol: bfd_get_arch_size() always returns -1 in dso__load_bfd_symbols()
--

commit 6d3220ac244766831d73c524c0b8f5184af28a7b
Author: Namhyung Kim <namhyung@kernel.org>

perf symbol: Ensure dso__set_is_64_bit() for libbfd

This commit calls dso__set_is_64_bit() using bfd_get_arch_size() when
loading symbols via libbfd in dso__load_bfd_symbols().

> diff --git a/tools/perf/util/libbfd.c b/tools/perf/util/libbfd.c
> index a4dcb406c79e9..9cc6216ce3adb 100644
> --- a/tools/perf/util/libbfd.c
> +++ b/tools/perf/util/libbfd.c
> @@ -462,6 +462,7 @@ int dso__load_bfd_symbols(struct dso *dso, const char *debugfile)
>  	symbols__fixup_end(dso__symbols(dso), false);
>  	symbols__fixup_duplicate(dso__symbols(dso));
>  	dso__set_adjust_symbols(dso, true);
> +	dso__set_is_64_bit(dso, bfd_get_arch_size(abfd) == 64);

[Severity: Medium]
Will bfd_get_arch_size(abfd) always return -1 here?

Earlier in tools/perf/util/libbfd.c:dso__load_bfd_symbols(), ELF binaries
are rejected before reaching this point:

	if (bfd_get_flavour(abfd) == bfd_target_elf_flavour)
		goto out_close;

In GNU binutils libbfd, bfd_get_arch_size() only checks ELF targets and
returns -1 for any other BFD flavour:

	if (abfd->xvec->flavour == bfd_target_elf_flavour)
		return get_elf_backend_data (abfd)->s->arch_size;
	return -1;

Because abfd is guaranteed to be non-ELF here (such as a 64-bit PE/COFF
binary), bfd_get_arch_size(abfd) == 64 always evaluates to false and
unconditionally sets dso->is_64_bit to false.

Should this use bfd_arch_bits_per_address(abfd) == 64 instead so that the
address size is queried across all BFD flavours?

>  
>  	err = dso__has_symbols(dso);
>  out_free:

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