Re: [PATCH v3 3/4] perf annotate: Be robust to annotating without a thread

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Capstone initialization bypasses the DSO architecture fallback when a thread is not provided, breaking cross-architecture disassembly.
- [Medium] Memory leak of dynamically allocated `filename` in `hist_entry__annotate_printf` when `map_symbol__get_arch` fails.
--

--- Patch [3]: [PATCH v3 3/4] perf annotate: Be robust to annotating without a thread ---
commit 73f48ecf5d22ea055ab30fc02eef31fb06587ac0
Author: Ian Rogers <irogers@google.com>

perf annotate: Be robust to annotating without a thread

If a thread isn't given to thread__get_arch try harder to determine
the arch for disassembly. Do this by passing an entire map_symbol and
using fallback paths such as reading the e_machine from a map's
dso. At the same time ensure some other uses of map_symbol thread
don't assume it is non-NULL to avoid segvs.

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c

[ ... ]

> @@ -1266,7 +1291,8 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
>  
>  	apd.addr_fmt_width = annotated_source__addr_fmt_width(&notes->src->source,
>  							      notes->src->start);
> -	thread__get_arch(ms->thread, &apd.arch);
> +	if (map_symbol__get_arch(ms, &apd.arch))
> +		return -ENOTSUP;

[Severity: Medium]
Does this code leak the allocated filename?

Earlier in hist_entry__annotate_printf(), filename is allocated dynamically:

filename = strdup(dso__long_name(dso));

But this newly added error path returns directly, bypassing the free(filename)
cleanup that happens at the end of the function when an unsupported
architecture is processed.

>  	apd.dbg = dso__debuginfo(dso);
>  
>  	list_for_each_entry(pos, &notes->src->source, node) {

[ ... ]

> diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> --- a/tools/perf/util/capstone.c
> +++ b/tools/perf/util/capstone.c
> @@ -414,6 +414,10 @@ int symbol__disassemble_capstone(const char *filename, struct symbol *sym,
>  	e_machine = thread__e_machine_endian(args->ms->thread,
>  					     /*machine=*/NULL,
>  					     /*e_flags=*/NULL, &is_big_endian);
> +	if (e_machine == EM_NONE && dso) {
> +		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	}

[Severity: High]
Will this fallback logic ever execute when the thread is NULL?

The commit message mentions falling back to the DSO architecture when a thread
is missing. However, if args->ms->thread is NULL, thread__e_machine_endian()
returns the host system's architecture (e.g., EM_X86_64) rather than EM_NONE.

Because e_machine is not EM_NONE, this newly added fallback check is bypassed,
and capstone is initialized with the host architecture. Can this break
cross-architecture analysis for threads without a valid machine?

>  	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
>  		goto err;

[ ... ]

> @@ -529,6 +533,10 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
>  	e_machine = thread__e_machine_endian(args->ms->thread,
>  					     /*machine=*/NULL,
>  					     /*e_flags=*/NULL, &is_big_endian);
> +	if (e_machine == EM_NONE && dso) {
> +		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	}

[Severity: High]
Is this the same fallback issue as above?

When args->ms->thread is NULL, thread__e_machine_endian() returns the host
architecture instead of EM_NONE, bypassing the dso fallback logic in the
powerpc implementation as well.

>  	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
>  		goto err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709165220.1411149-1-irogers@google.com?part=3