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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Capstone disassembly silently falls back to the host architecture when `ms->thread` is `NULL`, breaking cross-platform annotation.

Pre-existing issues:
- [High] Missing error handling for `map_symbol__get_arch()` causes a NULL pointer dereference crash when processing unsupported architectures.
- [High] Unconditional dereference of `result->name` when `result` is NULL in `arch__find()` error logging.
--

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

perf annotate: Be robust to annotating without a thread

The patch makes perf annotate more robust when annotating without a thread
context by falling back to reading the e_machine from a map's dso. It also
ensures other uses of map_symbol thread don't assume it is non-NULL.

> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 02505222d8c2d..e89029f0d0958 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -982,21 +982,46 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
[ ... ]
> -int thread__get_arch(struct thread *thread, const struct arch **parch)
> +int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch)
>  {
> +	struct machine *machine = NULL;
> +	const char *cpuid = NULL;
>  	const struct arch *arch;
> -	struct machine *machine;
> -	uint32_t e_flags;
> -	uint16_t e_machine;
[ ... ]
> -	machine = maps__machine(thread__maps(thread));
> -	e_machine = thread__e_machine(thread, machine, &e_flags);
> -	arch = arch__find(e_machine, e_flags, machine->env ? machine->env->cpuid : NULL);
> +	arch = arch__find(e_machine, e_flags, cpuid);

[Severity: High]
This is a pre-existing issue, but can this error path in arch__find() crash?

If arch_new_fn[e_machine]() fails (e.g., rejecting a malformed CPUID string)
and returns NULL, it sets result to NULL. The error handling path checks if
result is NULL, but then immediately dereferences result->name in the print
statement:

tools/perf/util/disasm.c:arch__find() {
	...
	result = arch_new_fn[e_machine](&key, cpuid);
	if (!result) {
		pr_err("%s: failed to initialize %s (%u) arch priv area\n",
			__func__, result->name, e_machine);
		free(tmp);
		return NULL;
	}
	...
}

>  	if (arch == NULL) {
>  		pr_err("%s: unsupported arch %d\n", __func__, e_machine);
>  		return errno;
>  	}
>  	if (parch)
>  		*parch = arch;
>  
>  	return 0;
>  }

[Severity: High]
With the introduction of map_symbol__get_arch() to dynamically determine the
architecture via the DSO when ms->thread is NULL, will capstone disassembly
still work correctly when cross-compiling?

In tools/perf/util/capstone.c:symbol__disassemble_capstone():

	e_machine = thread__e_machine_endian(args->ms->thread,
					     /*machine=*/NULL,
					     /*e_flags=*/NULL, &is_big_endian);
	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)

And similarly in symbol__disassemble_capstone_powerpc():

	e_machine = thread__e_machine_endian(args->ms->thread,
					     /*machine=*/NULL,
					     /*e_flags=*/NULL, &is_big_endian);

Both functions bypass map_symbol__get_arch() entirely. When annotating without
a thread context, ms->thread is NULL, which causes thread__e_machine_endian()
to fall back to the host machine's architecture instead of using the DSO. Can
this break cross-platform annotation for capstone?

> @@ -1266,7 +1291,7 @@ 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);
> +	map_symbol__get_arch(ms, &apd.arch);

[Severity: High]
This is a pre-existing issue, but should the return value of
map_symbol__get_arch() be checked here?

If map_symbol__get_arch() fails (e.g., when encountering an unsupported
architecture), it returns an error code and leaves apd.arch as NULL. This NULL
pointer is then passed through apd and eventually dereferenced:

annotation_line__print()
  __hist_entry__get_data_type()
    annotate_get_insn_location()
      arch__is_powerpc()

tools/perf/util/disasm.c:arch__is_powerpc() {
	return arch->id.e_machine == EM_PPC || arch->id.e_machine == EM_PPC64;
}

Can this lead to a segmentation fault when processing unsupported architectures?

>  	apd.dbg = dso__debuginfo(dso);
>  
>  	list_for_each_entry(pos, &notes->src->source, node) {
[ ... ]
> @@ -1371,7 +1396,7 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
>  	struct annotation_line *al;
>  
>  	if (annotate_opts.code_with_type) {
> -		thread__get_arch(apd->he->ms.thread, &apd->arch);
> +		map_symbol__get_arch(&apd->he->ms, &apd->arch);

[Severity: High]
This is a pre-existing issue, but this call also ignores the return value,
which could lead to the same NULL pointer dereference described above.

>  		apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
>  	}
>

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