[PATCH v1 1/3] perf ui hists: In report UI ensure thread is set

[PATCH v1 1/3] perf ui hists: In report UI ensure thread is set

[Ian Rogers]

A map_symbol contains a thread that provides access to the wider perf
session. Recent changes used the thread to get the e_machine to better
support cross-platform disassembly. Unfortunately the UI code wasn't
populating the thread and so the e_machine lookup failed early
breaking the 'a' for annotate TUI action. Populate the map_symbol
thread to fix this.

Reported-by: Josh Stone <jistone@redhat.com>
Fixes: 0e26ba5a8774 ("perf disasm: Refactor arch__find and initialization of arch structs")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index da7cc195b9f4..f8ffd9b73582 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 						continue;
 
 					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
+					actions->ms.thread = bi->to.ms.thread;
 					actions->ms.map = bi->to.ms.map;
 				} else {
 					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
 										 browser->selection->map);
+					actions->ms.thread = browser->selection->thread;
 					actions->ms.map = browser->selection->map;
 				}
 
@@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					continue;
 				}
 
+				actions->ms.thread = browser->selection->thread;
 				actions->ms.map = browser->selection->map;
 				actions->ms.sym = browser->selection->sym;
 			}
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action

[Ian Rogers]

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only
ever 1 thread with a popup_action.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f8ffd9b73582..c032068568d5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2455,7 +2455,6 @@ static int switch_data_file(void)
 
 struct popup_action {
 	unsigned long		time;
-	struct thread 		*thread;
 	int (*fn)(struct hist_browser *browser, struct popup_action *act);
 	struct map_symbol 	ms;
 	int			socket;
@@ -2572,7 +2571,7 @@ add_annotate_type_opt(struct popup_action *act, char **optstr,
 static int
 do_zoom_thread(struct hist_browser *browser, struct popup_action *act)
 {
-	struct thread *thread = act->thread;
+	struct thread *thread = act->ms.thread;
 
 	if ((!hists__has(browser->hists, thread) &&
 	     !hists__has(browser->hists, comm)) || thread == NULL)
@@ -2627,7 +2626,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
 	if (ret < 0)
 		return 0;
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->fn = do_zoom_thread;
 	return 1;
 }
@@ -2733,8 +2732,8 @@ do_run_script(struct hist_browser *browser,
 	int n = 0;
 
 	len = 100;
-	if (act->thread)
-		len += strlen(thread__comm_str(act->thread));
+	if (act->ms.thread)
+		len += strlen(thread__comm_str(act->ms.thread));
 	else if (act->ms.sym)
 		len += strlen(act->ms.sym->name);
 	script_opt = malloc(len);
@@ -2742,9 +2741,9 @@ do_run_script(struct hist_browser *browser,
 		return -1;
 
 	script_opt[0] = 0;
-	if (act->thread) {
+	if (act->ms.thread) {
 		n = scnprintf(script_opt, len, " -c %s ",
-			  thread__comm_str(act->thread));
+			  thread__comm_str(act->ms.thread));
 	} else if (act->ms.sym) {
 		n = scnprintf(script_opt, len, " -S %s ",
 			  act->ms.sym->name);
@@ -2799,7 +2798,7 @@ add_script_opt_2(struct popup_action *act, char **optstr,
 			return 0;
 	}
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->ms.sym = sym;
 	act->fn = do_run_script;
 	return 1;
@@ -3202,7 +3201,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					   verbose);
 			continue;
 		case 't':
-			actions->thread = thread;
+			actions->ms.thread = thread;
 			do_zoom_thread(browser, actions);
 			continue;
 		case 'S':
@@ -3222,7 +3221,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 			continue;
 		case 'r':
 			if (is_report_browser(hbt)) {
-				actions->thread = NULL;
+				actions->ms.thread = NULL;
 				actions->ms.sym = NULL;
 				do_run_script(browser, actions);
 			}
@@ -3305,7 +3304,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 				 */
 				do_zoom_dso(browser, actions);
 			} else if (top == &browser->hists->thread_filter) {
-				actions->thread = thread;
+				actions->ms.thread = thread;
 				do_zoom_thread(browser, actions);
 			} else if (top == &browser->hists->socket_filter) {
 				do_zoom_socket(browser, actions);
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action

[sashiko-bot]

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

Pre-existing issues:
- [High] Stack buffer overflow due to out-of-bounds writes on `actions` and `options` arrays.
--

--- Patch [2]: [PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action ---
commit 8f959fa67798b84a03f0c7cd56908afc452f4cc7
Author: Ian Rogers <irogers@google.com>

perf ui hists: Remove duplicated thread in popup_action

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only
ever 1 thread with a popup_action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 6353be9dba865..a93846b8dc885 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c

[ ... ]

> @@ -3305,7 +3304,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  				 */
>  				do_zoom_dso(browser, actions);
>  			} else if (top == &browser->hists->thread_filter) {
> -				actions->thread = thread;
> +				actions->ms.thread = thread;
>  				do_zoom_thread(browser, actions);
>  			} else if (top == &browser->hists->socket_filter) {
>  				do_zoom_socket(browser, actions);

[Severity: High]
This is a pre-existing issue, but can this code overflow the actions[] and options[] stack arrays later in evsel__hists_browse()?

The arrays are statically allocated on the stack with a maximum size of 16:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
	#define MAX_OPTIONS  16
	char *options[MAX_OPTIONS];
	struct popup_action actions[MAX_OPTIONS];
	...
}

Further down in the function, nr_options is repeatedly incremented without bounds checking:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
	...
	nr_options += add_annotate_opt(&actions[nr_options],
				       &options[nr_options],
				       &bi->from.ms,
				       bi->from.al_addr);
	...
}

If a user executes perf report with specific arguments on a crafted perf.data file, couldn't the number of options added exceed 16 and result in a stack buffer overflow?

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

[PATCH v1 3/3] perf annotate: Be robust to annotating without a thread

[Ian Rogers]

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.

Tested running perf report without a previous fix to populate the
map_symbol thread field.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/annotate.c |  2 +-
 tools/perf/util/annotate.c        | 51 +++++++++++++++++++++++--------
 tools/perf/util/annotate.h        |  2 +-
 tools/perf/util/disasm.c          |  7 +++--
 4 files changed, 44 insertions(+), 18 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d25761a8d25e..e47a46775089 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -1201,7 +1201,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
 				ui__warning("Annotation has no source code.");
 		}
 	} else {
-		err = thread__get_arch(ms->thread, &browser.arch);
+		err = map_symbol__get_arch(ms, &browser.arch);
 		if (err) {
 			annotate_browser__symbol_annotate_error(&browser, err);
 			return -1;
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 53b2a224b21d..82eee0f417e0 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)
 	annotation__calc_percent(notes, evsel, symbol__size(sym));
 }
 
-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;
+	uint32_t e_flags = 0;
+	uint16_t e_machine = EM_NONE;
 
-	if (!thread) {
-		*parch = NULL;
-		return -1;
+	/*
+	 * If we have a thread then derive the e_machine using it. The thread
+	 * can determine the machine and perf_env, so is generally pretty robust
+	 * as well as benefitting from a cache.
+	 */
+	if (ms->thread) {
+		struct maps *maps = thread__maps(ms->thread);
+
+		machine = maps__machine(maps);
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+		if (machine && machine->env)
+			cpuid = machine->env->cpuid;
+	}
+	if (e_machine == EM_NONE && ms->map) {
+		/*
+		 * If the thread look up failed then try to use a dso that may
+		 * be associated with the map.
+		 */
+		struct dso *dso = map__dso(ms->map);
+
+		if (dso)
+			e_machine = dso__e_machine(dso, machine, &e_flags);
+	}
+	if (e_machine == EM_NONE) {
+		/*
+		 * Still no e_machine, use the default for this kind of perf
+		 * build that will determine a value using uname, etc.
+		 */
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
 	}
 
-	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);
 	if (arch == NULL) {
 		pr_err("%s: unsupported arch %d\n", __func__, e_machine);
 		return errno;
@@ -1018,7 +1043,7 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
 	const struct arch *arch = NULL;
 	int err, nr;
 
-	err = thread__get_arch(ms->thread, &arch);
+	err = map_symbol__get_arch(ms, &arch);
 	if (err)
 		return err;
 
@@ -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);
 	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);
 		apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
 	}
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 1aa6df7d1618..d54f851035ec 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -584,5 +584,5 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr, u64 *br_cntr,
 			     int num_aggr, struct evsel *evsel);
 int annotation_br_cntr_abbr_list(char **str, struct evsel *evsel, bool header);
 
-int thread__get_arch(struct thread *thread, const struct arch **parch);
+int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch);
 #endif	/* __PERF_ANNOTATE_H */
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 0a1a7e9cf3ef..8cd90a0a0f8a 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -271,7 +271,7 @@ static int call__parse(const struct arch *arch, struct ins_operands *ops, struct
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -405,7 +405,7 @@ static int jump__parse(const struct arch *arch, struct ins_operands *ops, struct
 	 * Actual navigation will come next, with further understanding of how
 	 * the symbol searching and disassembly should be done.
 	 */
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -1068,7 +1068,8 @@ static int symbol__parse_objdump_line(struct symbol *sym,
 			.ms = { .map = map__get(map), },
 		};
 
-		if (!maps__find_ams(thread__maps(args->ms->thread), &target) &&
+		if (args->ms->thread &&
+		    !maps__find_ams(thread__maps(args->ms->thread), &target) &&
 		    target.ms.sym->start == target.al_addr)
 			dl->ops.target.sym = target.ms.sym;
 
-- 
2.55.0.795.g602f6c329a-goog

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Architecture-specific disassembly parsers for LoongArch and s390 dereference `ms->thread` without checking for NULL, causing segmentation faults when annotating without a thread.
--

--- Patch [3]: [PATCH v1 3/3] perf annotate: Be robust to annotating without a thread ---
commit 64d4504d663ab9115662531cbaf640b34642de60
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/disasm.c b/tools/perf/util/disasm.c
> index 0a1a7e9cf3efe..8cd90a0a0f8ae 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -271,7 +271,7 @@ static int call__parse(const struct arch *arch, struct ins_operands *ops, struct
>  		.addr = map__objdump_2mem(map, ops->target.addr),
>  	};
>  
> -	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
> +	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&

[Severity: Medium]
Since ms->thread is now allowed to be NULL, do the architecture-specific
parsers need similar updates?

For example, looking at loongarch_call__parse() in
tools/perf/util/annotate-arch/annotate-loongarch.c:

	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)

It appears to unconditionally call thread__maps(ms->thread). The same
pattern is also present in loongarch_jump__parse() and s390_call__parse().

Could this regression cause a segmentation fault when running perf annotate
without a thread on LoongArch or s390 binaries?

>  	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
>  		ops->target.sym = target.ms.sym;
>

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

[PATCH v2 1/4] perf ui hists: In report UI ensure thread is set

[Ian Rogers]

A map_symbol contains a thread that provides access to the wider perf
session. Recent changes used the thread to get the e_machine to better
support cross-platform disassembly. Unfortunately the UI code wasn't
populating the thread and so the e_machine lookup failed early
breaking the 'a' for annotate TUI action. Populate the map_symbol
thread to fix this.

Reported-by: Josh Stone <jistone@redhat.com>
Fixes: 0e26ba5a8774 ("perf disasm: Refactor arch__find and initialization of arch structs")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index da7cc195b9f4..f8ffd9b73582 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 						continue;
 
 					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
+					actions->ms.thread = bi->to.ms.thread;
 					actions->ms.map = bi->to.ms.map;
 				} else {
 					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
 										 browser->selection->map);
+					actions->ms.thread = browser->selection->thread;
 					actions->ms.map = browser->selection->map;
 				}
 
@@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					continue;
 				}
 
+				actions->ms.thread = browser->selection->thread;
 				actions->ms.map = browser->selection->map;
 				actions->ms.sym = browser->selection->sym;
 			}
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action

[Ian Rogers]

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only
ever 1 thread with a popup_action.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f8ffd9b73582..c032068568d5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2455,7 +2455,6 @@ static int switch_data_file(void)
 
 struct popup_action {
 	unsigned long		time;
-	struct thread 		*thread;
 	int (*fn)(struct hist_browser *browser, struct popup_action *act);
 	struct map_symbol 	ms;
 	int			socket;
@@ -2572,7 +2571,7 @@ add_annotate_type_opt(struct popup_action *act, char **optstr,
 static int
 do_zoom_thread(struct hist_browser *browser, struct popup_action *act)
 {
-	struct thread *thread = act->thread;
+	struct thread *thread = act->ms.thread;
 
 	if ((!hists__has(browser->hists, thread) &&
 	     !hists__has(browser->hists, comm)) || thread == NULL)
@@ -2627,7 +2626,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
 	if (ret < 0)
 		return 0;
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->fn = do_zoom_thread;
 	return 1;
 }
@@ -2733,8 +2732,8 @@ do_run_script(struct hist_browser *browser,
 	int n = 0;
 
 	len = 100;
-	if (act->thread)
-		len += strlen(thread__comm_str(act->thread));
+	if (act->ms.thread)
+		len += strlen(thread__comm_str(act->ms.thread));
 	else if (act->ms.sym)
 		len += strlen(act->ms.sym->name);
 	script_opt = malloc(len);
@@ -2742,9 +2741,9 @@ do_run_script(struct hist_browser *browser,
 		return -1;
 
 	script_opt[0] = 0;
-	if (act->thread) {
+	if (act->ms.thread) {
 		n = scnprintf(script_opt, len, " -c %s ",
-			  thread__comm_str(act->thread));
+			  thread__comm_str(act->ms.thread));
 	} else if (act->ms.sym) {
 		n = scnprintf(script_opt, len, " -S %s ",
 			  act->ms.sym->name);
@@ -2799,7 +2798,7 @@ add_script_opt_2(struct popup_action *act, char **optstr,
 			return 0;
 	}
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->ms.sym = sym;
 	act->fn = do_run_script;
 	return 1;
@@ -3202,7 +3201,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					   verbose);
 			continue;
 		case 't':
-			actions->thread = thread;
+			actions->ms.thread = thread;
 			do_zoom_thread(browser, actions);
 			continue;
 		case 'S':
@@ -3222,7 +3221,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 			continue;
 		case 'r':
 			if (is_report_browser(hbt)) {
-				actions->thread = NULL;
+				actions->ms.thread = NULL;
 				actions->ms.sym = NULL;
 				do_run_script(browser, actions);
 			}
@@ -3305,7 +3304,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 				 */
 				do_zoom_dso(browser, actions);
 			} else if (top == &browser->hists->thread_filter) {
-				actions->thread = thread;
+				actions->ms.thread = thread;
 				do_zoom_thread(browser, actions);
 			} else if (top == &browser->hists->socket_filter) {
 				do_zoom_socket(browser, actions);
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action

[sashiko-bot]

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

Pre-existing issues:
- [High] Unbounded addition of popup menu options leads to a stack buffer overflow.
--

--- Patch [2]: [PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action ---
commit 52ae50056f9a85a49a481078d9bef28046659aff
Author: Ian Rogers <irogers@google.com>

perf ui hists: Remove duplicated thread in popup_action

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only
ever 1 thread with a popup_action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 6353be9dba865..a93846b8dc885 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -3202,7 +3201,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					   verbose);
>  			continue;
>  		case 't':
> -			actions->thread = thread;
> +			actions->ms.thread = thread;

[Severity: High]
This isn't a bug introduced by this patch, but is there a potential stack
buffer overflow in evsel__hists_browse() when building the popup menu?

It looks like the options and actions arrays are defined with a fixed size:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    #define MAX_OPTIONS  16
    char *options[MAX_OPTIONS];
    struct popup_action actions[MAX_OPTIONS];
    ...

However, when interacting with the UI on complex profiling data, various
add_*_opt functions append elements and advance nr_options without bounds
checking:

    skip_annotation:
        nr_options += add_annotate_type_opt(&actions[nr_options], ...);
        nr_options += add_thread_opt(browser, &actions[nr_options], ...);
        ...
        if (hists__has(hists, sym) && browser->selection->sym) {
            nr_options += add_script_opt(browser, &actions[nr_options], ...);

Could this write out of bounds if the total number of added options exceeds 16?

>  			do_zoom_thread(browser, actions);
>  			continue;

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

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

[Ian Rogers]

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.

Tested running perf report without a previous fix to populate the
map_symbol thread field.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/annotate.c             |  2 +-
 .../util/annotate-arch/annotate-loongarch.c   |  4 +-
 tools/perf/util/annotate-arch/annotate-s390.c |  2 +-
 tools/perf/util/annotate.c                    | 51 ++++++++++++++-----
 tools/perf/util/annotate.h                    |  2 +-
 tools/perf/util/disasm.c                      |  7 +--
 6 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d25761a8d25e..e47a46775089 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -1201,7 +1201,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
 				ui__warning("Annotation has no source code.");
 		}
 	} else {
-		err = thread__get_arch(ms->thread, &browser.arch);
+		err = map_symbol__get_arch(ms, &browser.arch);
 		if (err) {
 			annotate_browser__symbol_annotate_error(&browser, err);
 			return -1;
diff --git a/tools/perf/util/annotate-arch/annotate-loongarch.c b/tools/perf/util/annotate-arch/annotate-loongarch.c
index c2addca77320..26217cb2d8f3 100644
--- a/tools/perf/util/annotate-arch/annotate-loongarch.c
+++ b/tools/perf/util/annotate-arch/annotate-loongarch.c
@@ -51,7 +51,7 @@ static int loongarch_call__parse(const struct arch *arch, struct ins_operands *o
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -95,7 +95,7 @@ static int loongarch_jump__parse(const struct arch *arch, struct ins_operands *o
 
 	ops->target.outside = target.addr < start || target.addr >= end;
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
diff --git a/tools/perf/util/annotate-arch/annotate-s390.c b/tools/perf/util/annotate-arch/annotate-s390.c
index af9cabd0a586..3ea6ad588998 100644
--- a/tools/perf/util/annotate-arch/annotate-s390.c
+++ b/tools/perf/util/annotate-arch/annotate-s390.c
@@ -50,7 +50,7 @@ static int s390_call__parse(const struct arch *arch, struct ins_operands *ops,
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 53b2a224b21d..82eee0f417e0 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)
 	annotation__calc_percent(notes, evsel, symbol__size(sym));
 }
 
-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;
+	uint32_t e_flags = 0;
+	uint16_t e_machine = EM_NONE;
 
-	if (!thread) {
-		*parch = NULL;
-		return -1;
+	/*
+	 * If we have a thread then derive the e_machine using it. The thread
+	 * can determine the machine and perf_env, so is generally pretty robust
+	 * as well as benefitting from a cache.
+	 */
+	if (ms->thread) {
+		struct maps *maps = thread__maps(ms->thread);
+
+		machine = maps__machine(maps);
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+		if (machine && machine->env)
+			cpuid = machine->env->cpuid;
+	}
+	if (e_machine == EM_NONE && ms->map) {
+		/*
+		 * If the thread look up failed then try to use a dso that may
+		 * be associated with the map.
+		 */
+		struct dso *dso = map__dso(ms->map);
+
+		if (dso)
+			e_machine = dso__e_machine(dso, machine, &e_flags);
+	}
+	if (e_machine == EM_NONE) {
+		/*
+		 * Still no e_machine, use the default for this kind of perf
+		 * build that will determine a value using uname, etc.
+		 */
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
 	}
 
-	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);
 	if (arch == NULL) {
 		pr_err("%s: unsupported arch %d\n", __func__, e_machine);
 		return errno;
@@ -1018,7 +1043,7 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
 	const struct arch *arch = NULL;
 	int err, nr;
 
-	err = thread__get_arch(ms->thread, &arch);
+	err = map_symbol__get_arch(ms, &arch);
 	if (err)
 		return err;
 
@@ -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);
 	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);
 		apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
 	}
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 1aa6df7d1618..d54f851035ec 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -584,5 +584,5 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr, u64 *br_cntr,
 			     int num_aggr, struct evsel *evsel);
 int annotation_br_cntr_abbr_list(char **str, struct evsel *evsel, bool header);
 
-int thread__get_arch(struct thread *thread, const struct arch **parch);
+int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch);
 #endif	/* __PERF_ANNOTATE_H */
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 0a1a7e9cf3ef..8cd90a0a0f8a 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -271,7 +271,7 @@ static int call__parse(const struct arch *arch, struct ins_operands *ops, struct
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -405,7 +405,7 @@ static int jump__parse(const struct arch *arch, struct ins_operands *ops, struct
 	 * Actual navigation will come next, with further understanding of how
 	 * the symbol searching and disassembly should be done.
 	 */
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -1068,7 +1068,8 @@ static int symbol__parse_objdump_line(struct symbol *sym,
 			.ms = { .map = map__get(map), },
 		};
 
-		if (!maps__find_ams(thread__maps(args->ms->thread), &target) &&
+		if (args->ms->thread &&
+		    !maps__find_ams(thread__maps(args->ms->thread), &target) &&
 		    target.ms.sym->start == target.al_addr)
 			dl->ops.target.sym = target.ms.sym;
 
-- 
2.55.0.795.g602f6c329a-goog

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

[sashiko-bot]

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

[PATCH v2 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[Ian Rogers]

In evsel__hists_browse(), the 'options' and 'actions' arrays are
statically allocated on the stack with a size of MAX_OPTIONS
(16). Further down, the function sequentially calls several
add_*_opt() functions, which increment nr_options without bounds
checking.

Depending on the context (e.g., branch mode, scripting, annotations),
the sum of added options can theoretically exceed 16 (potentially
reaching up to ~19).  This could lead to a stack buffer overflow.

Increase MAX_OPTIONS to 32 to accommodate the maximum possible number
of options without risking an overflow.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index c032068568d5..ff607e36a0a6 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3002,7 +3002,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
 	struct branch_info *bi = NULL;
-#define MAX_OPTIONS  16
+#define MAX_OPTIONS  32
 	char *options[MAX_OPTIONS];
 	struct popup_action actions[MAX_OPTIONS];
 	int nr_options = 0;
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v2 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[sashiko-bot]

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

Pre-existing issues:
- [High] Uninitialized stack memory passed to free() on pstack__new() error path, causing heap corruption.
--

--- Patch [4]: [PATCH v2 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow ---
commit a3310ba19dbf043afaf000421ada085b9b1e380f
Author: Ian Rogers <irogers@google.com>

perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

In evsel__hists_browse(), the 'options' and 'actions' arrays are statically
allocated on the stack with a size of MAX_OPTIONS (16). Depending on the
context, the sum of added options can theoretically exceed 16 (potentially
reaching up to ~19). This could lead to a stack buffer overflow, so this
patch increases MAX_OPTIONS to 32 to accommodate the maximum possible number
of options without risking an overflow.

Closes: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index a93846b8dc885..2b81217b02dfd 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3002,7 +3002,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	struct hists *hists = evsel__hists(evsel);
>  	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
>  	struct branch_info *bi = NULL;
> -#define MAX_OPTIONS  16
> +#define MAX_OPTIONS  32
>  	char *options[MAX_OPTIONS];

[Severity: High]
This isn't a bug introduced by this patch, but if pstack__new() fails before
the array is initialized:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
	browser->pstack = pstack__new(3);
	if (browser->pstack == NULL)
		goto out;

	ui_helpline__push(helpline);

	memset(options, 0, sizeof(options));
    ...
out:
	hist_browser__delete(browser);
	free_popup_options(options, MAX_OPTIONS);
    ...
}

Does this pass uninitialized stack memory to free_popup_options(), which could
cause heap corruption when it attempts to free garbage stack pointers?

>  	struct popup_action actions[MAX_OPTIONS];
>  	int nr_options = 0;

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

Re: [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] Raw pointer assignment of reference-counted `struct thread` bypasses lifecycle helpers.
--

--- Patch [1]: [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set ---
commit 62729234032d27123f3a006298cb693ab486e133
Author: Ian Rogers <irogers@google.com>

perf ui hists: In report UI ensure thread is set

This commit populates the map_symbol thread in the UI code to fix the
e_machine lookup for cross-platform disassembly when using the annotate
action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  						continue;
>  
>  					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
> +					actions->ms.thread = bi->to.ms.thread;

[Severity: Low]
This isn't a bug, but should this assignment use the thread__get() lifecycle
helper? I noticed a raw pointer assignment for the reference-counted thread
struct here.

>  					actions->ms.map = bi->to.ms.map;
>  				} else {
>  					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
>  										 browser->selection->map);
> +					actions->ms.thread = browser->selection->thread;

[Severity: Low]
Should this use thread__get() as well?

>  					actions->ms.map = browser->selection->map;
>  				}
>  
> @@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					continue;
>  				}
>  
> +				actions->ms.thread = browser->selection->thread;

[Severity: Low]
Should this also use thread__get() instead of a raw pointer assignment?

>  				actions->ms.map = browser->selection->map;
>  				actions->ms.sym = browser->selection->sym;
>  			}

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

[PATCH v3 1/4] perf ui hists: In report UI ensure thread is set

[Ian Rogers]

A map_symbol contains a thread that provides access to the wider perf
session. Recent changes used the thread to get the e_machine to better
support cross-platform disassembly. Unfortunately the UI code wasn't
populating the thread and so the e_machine lookup failed early
breaking the 'a' for annotate TUI action. Populate the map_symbol
thread to fix this.

Note: For the popup_action array, we intentionally do NOT use
thread__get() or map__get() when assigning to ms.thread and
ms.map. The actions array has a strictly bounded lifecycle that does
not outlive the menu loop, making raw pointers safe. If we took
references, we would have to clean them up (e.g., via
map_symbol__exit()), which if erroneously applied to raw pointers
would incorrectly drop references owned by the caller and lead to a
use-after-free.

Reported-by: Josh Stone <jistone@redhat.com>
Fixes: 0e26ba5a8774 ("perf disasm: Refactor arch__find and initialization of arch structs")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index da7cc195b9f4..f8ffd9b73582 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 						continue;
 
 					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
+					actions->ms.thread = bi->to.ms.thread;
 					actions->ms.map = bi->to.ms.map;
 				} else {
 					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
 										 browser->selection->map);
+					actions->ms.thread = browser->selection->thread;
 					actions->ms.map = browser->selection->map;
 				}
 
@@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					continue;
 				}
 
+				actions->ms.thread = browser->selection->thread;
 				actions->ms.map = browser->selection->map;
 				actions->ms.sym = browser->selection->sym;
 			}
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action

[Ian Rogers]

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only
ever 1 thread with a popup_action.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f8ffd9b73582..c032068568d5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2455,7 +2455,6 @@ static int switch_data_file(void)
 
 struct popup_action {
 	unsigned long		time;
-	struct thread 		*thread;
 	int (*fn)(struct hist_browser *browser, struct popup_action *act);
 	struct map_symbol 	ms;
 	int			socket;
@@ -2572,7 +2571,7 @@ add_annotate_type_opt(struct popup_action *act, char **optstr,
 static int
 do_zoom_thread(struct hist_browser *browser, struct popup_action *act)
 {
-	struct thread *thread = act->thread;
+	struct thread *thread = act->ms.thread;
 
 	if ((!hists__has(browser->hists, thread) &&
 	     !hists__has(browser->hists, comm)) || thread == NULL)
@@ -2627,7 +2626,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
 	if (ret < 0)
 		return 0;
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->fn = do_zoom_thread;
 	return 1;
 }
@@ -2733,8 +2732,8 @@ do_run_script(struct hist_browser *browser,
 	int n = 0;
 
 	len = 100;
-	if (act->thread)
-		len += strlen(thread__comm_str(act->thread));
+	if (act->ms.thread)
+		len += strlen(thread__comm_str(act->ms.thread));
 	else if (act->ms.sym)
 		len += strlen(act->ms.sym->name);
 	script_opt = malloc(len);
@@ -2742,9 +2741,9 @@ do_run_script(struct hist_browser *browser,
 		return -1;
 
 	script_opt[0] = 0;
-	if (act->thread) {
+	if (act->ms.thread) {
 		n = scnprintf(script_opt, len, " -c %s ",
-			  thread__comm_str(act->thread));
+			  thread__comm_str(act->ms.thread));
 	} else if (act->ms.sym) {
 		n = scnprintf(script_opt, len, " -S %s ",
 			  act->ms.sym->name);
@@ -2799,7 +2798,7 @@ add_script_opt_2(struct popup_action *act, char **optstr,
 			return 0;
 	}
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->ms.sym = sym;
 	act->fn = do_run_script;
 	return 1;
@@ -3202,7 +3201,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					   verbose);
 			continue;
 		case 't':
-			actions->thread = thread;
+			actions->ms.thread = thread;
 			do_zoom_thread(browser, actions);
 			continue;
 		case 'S':
@@ -3222,7 +3221,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 			continue;
 		case 'r':
 			if (is_report_browser(hbt)) {
-				actions->thread = NULL;
+				actions->ms.thread = NULL;
 				actions->ms.sym = NULL;
 				do_run_script(browser, actions);
 			}
@@ -3305,7 +3304,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 				 */
 				do_zoom_dso(browser, actions);
 			} else if (top == &browser->hists->thread_filter) {
-				actions->thread = thread;
+				actions->ms.thread = thread;
 				do_zoom_thread(browser, actions);
 			} else if (top == &browser->hists->socket_filter) {
 				do_zoom_socket(browser, actions);
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action

[sashiko-bot]

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

Pre-existing issues:
- [High] Stack buffer overflow in `actions` and `options` arrays due to unbounded option additions.
--

--- Patch [2]: [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action ---
commit 72bd59b6c2f43a82a793219b81133029ab0f3b61
Author: Ian Rogers <irogers@google.com>

perf ui hists: Remove duplicated thread in popup_action

This commit removes the thread field from struct popup_action as it is
duplicated in the map_symbol structure. This ensures there is only ever
one thread associated with a popup_action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 6353be9dba865..a93846b8dc885 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c

[ ... ]

> @@ -3305,7 +3304,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  				 */
>  				do_zoom_dso(browser, actions);
>  			} else if (top == &browser->hists->thread_filter) {
> -				actions->thread = thread;
> +				actions->ms.thread = thread;
>  			} else if (top == &browser->hists->socket_filter) {
>  				do_zoom_socket(browser, actions);

[Severity: High]
This isn't a bug introduced by this patch, but looking at how the actions
array is populated in evsel__hists_browse(), is there a potential stack buffer
overflow here?

The options and actions arrays are statically sized to 16 elements on the
stack:

    #define MAX_OPTIONS  16
    char *options[MAX_OPTIONS];
    struct popup_action actions[MAX_OPTIONS];

Later in the same function, under the skip_annotation label, the code
unconditionally appends menu options using a sequence of calls like:

    nr_options += add_annotate_type_opt(&actions[nr_options], ...);
    nr_options += add_thread_opt(browser, &actions[nr_options], ...);
    nr_options += add_dso_opt(browser, &actions[nr_options], ...);

Depending on the flags and data available in the perf.data file (such as
data type annotations, threads, callchains, and time sorting), couldn't this
sequence of additions append up to 19 options, exceeding the 16-element limit?

Because the options and actions arrays are allocated adjacently on the stack,
could writing an asprintf string pointer past the end of the options array
directly overwrite the fn function pointer of actions[0]? 

If the user then selects the first menu option, could this result in a control
flow hijack when the program dereferences and jumps to the heap string pointer?

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

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

[Ian Rogers]

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.

Tested running perf report without a previous fix to populate the
map_symbol thread field.

Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/annotate.c             |  2 +-
 .../util/annotate-arch/annotate-loongarch.c   |  4 +-
 tools/perf/util/annotate-arch/annotate-s390.c |  2 +-
 tools/perf/util/annotate.c                    | 55 ++++++++++++++-----
 tools/perf/util/annotate.h                    |  2 +-
 tools/perf/util/capstone.c                    |  8 +++
 tools/perf/util/disasm.c                      | 11 ++--
 7 files changed, 61 insertions(+), 23 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d25761a8d25e..e47a46775089 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -1201,7 +1201,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
 				ui__warning("Annotation has no source code.");
 		}
 	} else {
-		err = thread__get_arch(ms->thread, &browser.arch);
+		err = map_symbol__get_arch(ms, &browser.arch);
 		if (err) {
 			annotate_browser__symbol_annotate_error(&browser, err);
 			return -1;
diff --git a/tools/perf/util/annotate-arch/annotate-loongarch.c b/tools/perf/util/annotate-arch/annotate-loongarch.c
index c2addca77320..26217cb2d8f3 100644
--- a/tools/perf/util/annotate-arch/annotate-loongarch.c
+++ b/tools/perf/util/annotate-arch/annotate-loongarch.c
@@ -51,7 +51,7 @@ static int loongarch_call__parse(const struct arch *arch, struct ins_operands *o
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -95,7 +95,7 @@ static int loongarch_jump__parse(const struct arch *arch, struct ins_operands *o
 
 	ops->target.outside = target.addr < start || target.addr >= end;
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
diff --git a/tools/perf/util/annotate-arch/annotate-s390.c b/tools/perf/util/annotate-arch/annotate-s390.c
index af9cabd0a586..3ea6ad588998 100644
--- a/tools/perf/util/annotate-arch/annotate-s390.c
+++ b/tools/perf/util/annotate-arch/annotate-s390.c
@@ -50,7 +50,7 @@ static int s390_call__parse(const struct arch *arch, struct ins_operands *ops,
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 53b2a224b21d..061b5cb9de8a 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)
 	annotation__calc_percent(notes, evsel, symbol__size(sym));
 }
 
-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;
+	uint32_t e_flags = 0;
+	uint16_t e_machine = EM_NONE;
 
-	if (!thread) {
-		*parch = NULL;
-		return -1;
+	/*
+	 * If we have a thread then derive the e_machine using it. The thread
+	 * can determine the machine and perf_env, so is generally pretty robust
+	 * as well as benefitting from a cache.
+	 */
+	if (ms->thread) {
+		struct maps *maps = thread__maps(ms->thread);
+
+		machine = maps__machine(maps);
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+		if (machine && machine->env)
+			cpuid = machine->env->cpuid;
 	}
+	if (e_machine == EM_NONE && ms->map) {
+		/*
+		 * If the thread look up failed then try to use a dso that may
+		 * be associated with the map.
+		 */
+		struct dso *dso = map__dso(ms->map);
 
-	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);
+		if (dso)
+			e_machine = dso__e_machine(dso, machine, &e_flags);
+	}
+	if (e_machine == EM_NONE) {
+		/*
+		 * Still no e_machine, use the default for this kind of perf
+		 * build that will determine a value using uname, etc.
+		 */
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+	}
+
+	arch = arch__find(e_machine, e_flags, cpuid);
 	if (arch == NULL) {
 		pr_err("%s: unsupported arch %d\n", __func__, e_machine);
 		return errno;
@@ -1018,7 +1043,7 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
 	const struct arch *arch = NULL;
 	int err, nr;
 
-	err = thread__get_arch(ms->thread, &arch);
+	err = map_symbol__get_arch(ms, &arch);
 	if (err)
 		return err;
 
@@ -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;
 	apd.dbg = dso__debuginfo(dso);
 
 	list_for_each_entry(pos, &notes->src->source, node) {
@@ -1369,9 +1395,12 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
 		.write_graph		 = FILE__write_graph,
 	};
 	struct annotation_line *al;
+	int err;
 
 	if (annotate_opts.code_with_type) {
-		thread__get_arch(apd->he->ms.thread, &apd->arch);
+		err = map_symbol__get_arch(&apd->he->ms, &apd->arch);
+		if (err)
+			return err;
 		apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
 	}
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 1aa6df7d1618..d54f851035ec 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -584,5 +584,5 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr, u64 *br_cntr,
 			     int num_aggr, struct evsel *evsel);
 int annotation_br_cntr_abbr_list(char **str, struct evsel *evsel, bool header);
 
-int thread__get_arch(struct thread *thread, const struct arch **parch);
+int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch);
 #endif	/* __PERF_ANNOTATE_H */
diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
index 5ad537fea436..c2b776c6982d 100644
--- 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);
+	}
 	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);
+	}
 	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
 		goto err;
 
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 0a1a7e9cf3ef..46b9c292d5eb 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -182,8 +182,8 @@ const struct arch *arch__find(uint16_t e_machine, uint32_t e_flags, const char *
 
 	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);
+		pr_err("%s: failed to initialize %u arch priv area\n",
+			__func__, e_machine);
 		free(tmp);
 		return NULL;
 	}
@@ -271,7 +271,7 @@ static int call__parse(const struct arch *arch, struct ins_operands *ops, struct
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -405,7 +405,7 @@ static int jump__parse(const struct arch *arch, struct ins_operands *ops, struct
 	 * Actual navigation will come next, with further understanding of how
 	 * the symbol searching and disassembly should be done.
 	 */
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -1068,7 +1068,8 @@ static int symbol__parse_objdump_line(struct symbol *sym,
 			.ms = { .map = map__get(map), },
 		};
 
-		if (!maps__find_ams(thread__maps(args->ms->thread), &target) &&
+		if (args->ms->thread &&
+		    !maps__find_ams(thread__maps(args->ms->thread), &target) &&
 		    target.ms.sym->start == target.al_addr)
 			dl->ops.target.sym = target.ms.sym;
 
-- 
2.55.0.795.g602f6c329a-goog

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

[sashiko-bot]

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

[PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[Ian Rogers]

In evsel__hists_browse(), the 'options' and 'actions' arrays are
statically allocated on the stack with a size of MAX_OPTIONS
(16). Further down, the function sequentially calls several
add_*_opt() functions, which increment nr_options without bounds
checking.

Depending on the context (e.g., branch mode, scripting, annotations),
the sum of added options can theoretically exceed 16 (potentially
reaching up to ~19).  This could lead to a stack buffer overflow.

Increase MAX_OPTIONS to 32 to accommodate the maximum possible number
of options without risking an overflow.

Sashiko also caught an existing bug where uninitialized memory could
be read on an error path. Move the memsets to avoid this.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index c032068568d5..3bd84a35df41 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3002,7 +3002,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
 	struct branch_info *bi = NULL;
-#define MAX_OPTIONS  16
+#define MAX_OPTIONS  32
 	char *options[MAX_OPTIONS];
 	struct popup_action actions[MAX_OPTIONS];
 	int nr_options = 0;
@@ -3063,15 +3063,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 		browser->min_pcnt = min_pcnt;
 	hist_browser__update_nr_entries(browser);
 
+	memset(options, 0, sizeof(options));
+	memset(actions, 0, sizeof(actions));
+
 	browser->pstack = pstack__new(3);
 	if (browser->pstack == NULL)
 		goto out;
 
 	ui_helpline__push(helpline);
 
-	memset(options, 0, sizeof(options));
-	memset(actions, 0, sizeof(actions));
-
 	if (symbol_conf.col_width_list_str)
 		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
 
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Memory leak of dynamically allocated popup menu strings in the interaction loop.
- [Medium] Missing reference counting for `dso_filter` can lead to a use-after-free.
- [Medium] Unconditional pointer increment after string allocation failure leads to a NULL pointer dereference.
--

--- Patch [4]: [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow ---
commit b8ca9e63522e300d76cedfda674dbc631afbc3e2
Author: Ian Rogers <irogers@google.com>

perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

The commit increases MAX_OPTIONS in evsel__hists_browse() from 16 to 32 to
prevent a potential stack buffer overflow when adding menu options. It also
moves the memset of options and actions earlier in the function to avoid
reading uninitialized memory on an error path.

Closes: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index a93846b8dc885..8e54c29728d1c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3002,7 +3002,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	struct hists *hists = evsel__hists(evsel);
>  	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
>  	struct branch_info *bi = NULL;
> -#define MAX_OPTIONS  16
> +#define MAX_OPTIONS  32
>  	char *options[MAX_OPTIONS];
>  	struct popup_action actions[MAX_OPTIONS];
>  	int nr_options = 0;
> @@ -3063,15 +3063,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  		browser->min_pcnt = min_pcnt;
>  	hist_browser__update_nr_entries(browser);
>  
> +	memset(options, 0, sizeof(options));
> +	memset(actions, 0, sizeof(actions));
> +
>  	browser->pstack = pstack__new(3);
>  	if (browser->pstack == NULL)
>  		goto out;
>  
>  	ui_helpline__push(helpline);
>  
> -	memset(options, 0, sizeof(options));
> -	memset(actions, 0, sizeof(actions));
> -
>  	if (symbol_conf.col_width_list_str)
>  		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
>  

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential memory leak in
evsel__hists_browse() later in the same function when handling the options
array.

When the interactive loop runs, the dynamically allocated strings added to
the options array via asprintf() appear to be abandoned because nr_options
is reset to 0 in each iteration without freeing them:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
    while (1) {
        ...
        key = 0; // reset key
do_hotkey:       // key came straight from options ui__popup_menu()
        choice = nr_options = 0;
    ...
}

Does this cause memory to leak continuously as the user interacts with the
menu and new strings are allocated?


[Severity: Medium]
This is another pre-existing issue, but I noticed a potential missing
reference count in hists_browser__zoom_map().

The reference-counted dso struct is assigned as a raw pointer without
acquiring a reference via dso__get():

tools/perf/ui/browsers/hists.c:hists_browser__zoom_map() {
    ...
    browser->hists->dso_filter = dso;
    ...
}

Could this lead to a use-after-free defect if the underlying dso is freed
while the filter remains active, since it's later cleared without calling
dso__put()?


[Severity: Medium]
This is also a pre-existing issue, but I found a sequence in add_script_opt()
that might cause a NULL pointer dereference.

If the first call to add_script_opt_2() fails (for example, due to an
asprintf allocation failure), the pointer remains unset. The function then
unconditionally increments the optstr and act pointers:

tools/perf/ui/browsers/hists.c:add_script_opt() {
    ...
    optstr++;
    act++;
    ...
}

If a second allocation then completes successfully, the caller increments
nr_options, but the options array would contain an uninitialized or NULL
pointer gap at its first active index.

Can this NULL pointer be subsequently dereferenced by ui__popup_menu() via
ui_browser__write_nstring()?

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

Re: [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Stack buffer overflow in `evsel__hists_browse()` due to unbounded popup menu options
--

--- Patch [1]: [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set ---
commit 6b0cc8c7dd2c933b2c9f1bf775218027677c766a
Author: Ian Rogers <irogers@google.com>

perf ui hists: In report UI ensure thread is set

This commit populates the thread field in map_symbol within the hists browser
UI to fix cross-platform disassembly. It intentionally uses raw pointers for
thread and map to avoid reference counting issues tied to the bounded lifecycle
of the actions array.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  						continue;
>  
>  					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
> +					actions->ms.thread = bi->to.ms.thread;
>  					actions->ms.map = bi->to.ms.map;
>  				} else {
>  					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
>  										 browser->selection->map);
> +					actions->ms.thread = browser->selection->thread;
>  					actions->ms.map = browser->selection->map;
>  				}
>  
> @@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					continue;
>  				}
>  
> +				actions->ms.thread = browser->selection->thread;

[Severity: Critical]
This is a pre-existing issue, but does the unbounded accumulation of options
risk overflowing the actions[] and options[] arrays on the stack?

Looking at evsel__hists_browse(), the arrays are fixed to MAX_OPTIONS (16):

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
#define MAX_OPTIONS  16
    char *options[MAX_OPTIONS];
    struct popup_action actions[MAX_OPTIONS];
    int nr_options = 0;

Later in the function when building the popup menu, there is a long sequence
of add_*_opt() calls without any bounds checking on nr_options:

    nr_options += add_annotate_opt(&actions[nr_options], ...);
    ...
    nr_options += add_annotate_type_opt(&actions[nr_options], ...);
    nr_options += add_thread_opt(browser, &actions[nr_options], ...);
    nr_options += add_dso_opt(browser, &actions[nr_options], ...);
    nr_options += add_callchain_toggle_opt(browser, &actions[nr_options], ...);
    nr_options += add_map_opt(browser, &actions[nr_options], ...);
    nr_options += add_socket_opt(browser, &actions[nr_options], ...);

If enough of these conditions are met simultaneously (e.g. branch annotations,
type, thread, DSO, callchain, map, socket, and multiple scripts), couldn't
nr_options exceed 16 and corrupt the stack?

>  				actions->ms.map = browser->selection->map;
>  				actions->ms.sym = browser->selection->sym;
>  			}

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

[PATCH v4 1/9] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[Ian Rogers]

In evsel__hists_browse(), the 'options' and 'actions' arrays are
statically allocated on the stack with a size of MAX_OPTIONS (16).
Further down, the function sequentially calls several add_*_opt()
functions, which increment nr_options without bounds checking.

Depending on the context (e.g., branch mode, scripting, annotations),
the sum of added options can theoretically exceed 16 (potentially
reaching up to ~19). This could lead to a stack buffer overflow.

Increase MAX_OPTIONS to 32 to safely accommodate the maximum possible
number of options without risking an overflow.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index da7cc195b9f4..6163cc3ace27 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3003,7 +3003,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
 	struct branch_info *bi = NULL;
-#define MAX_OPTIONS  16
+#define MAX_OPTIONS  32
 	char *options[MAX_OPTIONS];
 	struct popup_action actions[MAX_OPTIONS];
 	int nr_options = 0;
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v4 2/9] perf ui hists: In report UI ensure thread is set

[Ian Rogers]

This commit populates the map_symbol thread in the UI code to fix the
e_machine lookup for cross-platform disassembly when using the annotate
action.

(Note regarding ui/browsers/hists.c: For the popup_action array, we
intentionally do NOT use thread__get() or map__get() when assigning to
ms.thread and ms.map. The actions array has a strictly bounded lifecycle
that does not outlive the menu loop, making raw pointers safe. If we
took references, we would have to clean them up (e.g., via
map_symbol__exit()), which if erroneously applied to raw pointers would
incorrectly drop references owned by the caller and lead to a use-after-
free.)

Fixes: 0e26ba5a8774 ("perf disasm: Refactor arch__find and initialization of arch structs")
Reported-by: Josh Stone <jistone@redhat.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6163cc3ace27..6edea27f4f30 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 						continue;
 
 					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
+					actions->ms.thread = bi->to.ms.thread;
 					actions->ms.map = bi->to.ms.map;
 				} else {
 					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
 										 browser->selection->map);
+					actions->ms.thread = browser->selection->thread;
 					actions->ms.map = browser->selection->map;
 				}
 
@@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					continue;
 				}
 
+				actions->ms.thread = browser->selection->thread;
 				actions->ms.map = browser->selection->map;
 				actions->ms.sym = browser->selection->sym;
 			}
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v4 2/9] perf ui hists: In report UI ensure thread is set

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The commit intentionally uses raw pointer assignments for reference-counted structs, violating the subsystem's explicit lifecycle guidelines.
--

--- Patch [2]: [PATCH v4 2/9] perf ui hists: In report UI ensure thread is set ---
commit bd1f6d1690697868f7f32902eeee4100a0837b56
Author: Ian Rogers <irogers@google.com>

perf ui hists: In report UI ensure thread is set

This commit populates the map_symbol thread in the UI code to fix the
e_machine lookup for cross-platform disassembly when using the annotate
action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 671961946d56c..c6cfbcc580617 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  						continue;
>  
>  					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
> +					actions->ms.thread = bi->to.ms.thread;
> +					actions->ms.map = bi->to.ms.map;

[Severity: Medium]
The commit message notes the intentional use of raw pointers to avoid having
to clean up references. However, does this violate the perf subsystem
guidelines which strictly require using explicit thread__get() and
thread__put() lifecycle helpers for reference-counted structs like thread
and map? 

Could bypassing the reference counting abstraction here make the code more
fragile to use-after-free defects if the lifecycle of actions->ms.thread is
modified in the future?

>  				} else {
>  					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
>  										 browser->selection->map);
> +					actions->ms.thread = browser->selection->thread;
> +					actions->ms.map = browser->selection->map;
>  				}

[ ... ]

> @@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					continue;
>  				}
>  
> +				actions->ms.thread = browser->selection->thread;
>  				actions->ms.map = browser->selection->map;
>  				actions->ms.sym = browser->selection->sym;
>  			}

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

[PATCH v4 3/9] perf disasm: Fix potential NULL pointer dereference in arch__find()

[Ian Rogers]

If arch_new_fn[e_machine]() fails (e.g., returning NULL to reject an
architecture or upon allocation failure), the error path attempts to
print result->name, dereferencing a NULL pointer and leading to a
segmentation fault. Fix it by printing the numeric e_machine value
instead.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260709035721.9EE901F000E9@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/disasm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 0a1a7e9cf3ef..5b114f089d56 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -182,8 +182,8 @@ const struct arch *arch__find(uint16_t e_machine, uint32_t e_flags, const char *
 
 	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);
+		pr_err("%s: failed to initialize %u arch priv area\n",
+			__func__, e_machine);
 		free(tmp);
 		return NULL;
 	}
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v4 3/9] perf disasm: Fix potential NULL pointer dereference in arch__find()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Fixing the NULL pointer dereference unmasks a reachable Use-After-Free in `arch__find()` when `arch_new_fn[]` fails, as the error path frees the `tmp` array but leaves the static `archs` pointer dangling.
--

--- Patch [3]: [PATCH v4 3/9] perf disasm: Fix potential NULL pointer dereference in arch__find() ---
commit 709c36ab73e377d0cda7f8eb4df8b0ebdb9717c8
Author: Ian Rogers <irogers@google.com>

perf disasm: Fix potential NULL pointer dereference in arch__find()

This commit fixes a NULL pointer dereference in the error path of
arch__find(). It resolves a segmentation fault that occurs when
arch_new_fn[] returns NULL by printing the numeric e_machine value
instead of attempting to access result->name.

Closes: https://lore.kernel.org/linux-perf-users/20260709035721.9EE901F000E9@smtp.kernel.org/

> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 0a1a7e9cf3efe..5b114f089d560 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
> @@ -182,8 +182,8 @@ const struct arch *arch__find(uint16_t e_machine, uint32_t e_flags, const char *
>  
>  	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);
> +		pr_err("%s: failed to initialize %u arch priv area\n",
> +			__func__, e_machine);
>  		free(tmp);

[Severity: High]
Will this error path leave the static global archs pointer dangling?

Earlier in arch__find(), tmp is allocated via:
tmp = reallocarray(archs, num_archs + 1, sizeof(*archs));

If reallocarray() moves the memory block, the old memory pointed to by archs
is freed. If arch_new_fn() then fails, this error path calls free(tmp).
Because archs is only updated on success later in the function, it will
remain pointing to freed memory.

Now that the NULL pointer dereference crash is fixed, execution can successfully
return from this error path. If num_archs > 0, wouldn't a subsequent call to
arch__find() pass the dangling archs pointer into bsearch(), causing a
use-after-free?

>  		return NULL;
>  	}

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

[PATCH v4 4/9] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

[Ian Rogers]

In evsel__hists_browse(), if browser->pstack allocation fails, the
function jumps to the 'out:' cleanup label which invokes
free_popup_options(options, MAX_OPTIONS). Because the options array is
allocated on the stack and wasn't initialized until after the pstack
check, this passes uninitialized stack pointers to free(), causing heap
corruption.

Fix it by moving the memset() of options and actions to the beginning of
the setup sequence, before the first potential error bailout.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260709035230.6DBEE1F000E9@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6edea27f4f30..7842f228435d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3064,15 +3064,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 		browser->min_pcnt = min_pcnt;
 	hist_browser__update_nr_entries(browser);
 
+	memset(options, 0, sizeof(options));
+	memset(actions, 0, sizeof(actions));
+
 	browser->pstack = pstack__new(3);
 	if (browser->pstack == NULL)
 		goto out;
 
 	ui_helpline__push(helpline);
 
-	memset(options, 0, sizeof(options));
-	memset(actions, 0, sizeof(actions));
-
 	if (symbol_conf.col_width_list_str)
 		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
 
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v4 4/9] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [High] Stack Use-After-Free (Dangling Pointer) in symbol filter string.
--

--- Patch [4]: [PATCH v4 4/9] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure ---
commit afaaff3407afa197242fdd0b413eb9b246beadbe
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

Fixes heap corruption by initializing the options and actions arrays before
the pstack allocation check, preventing an uninitialized stack pointer from
being passed to free_popup_options() if the allocation fails.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260709035230.6DBEE1F000E9@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index c6cfbcc580617..8b4c7510991f3 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3064,15 +3064,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  		browser->min_pcnt = min_pcnt;
>  	hist_browser__update_nr_entries(browser);
>  

[Severity: High]
This is a pre-existing issue, but I noticed a potential stack use-after-free
involving the local variable buf that is defined in evsel__hists_browse().

Later in evsel__hists_browse(), the local stack array buf is assigned to
the persistent hists->symbol_filter_str:

hists->symbol_filter_str = *buf ? buf : NULL;

If the user presses TAB to switch to another event, evsel__hists_browse()
returns and pops its stack frame.

Can this dangling pointer be accessed by the background timer? It looks
like perf_top__sort_new_samples() periodically iterates over events and
calls hists__filter_entry_by_symbol(), which then dereferences the pointer:

tools/perf/util/hist.c:hists__filter_entry_by_symbol() {
    ...
    if (hists->symbol_filter_str != NULL &&
        (!he->ms.sym || strstr(he->ms.sym->name,
                   hists->symbol_filter_str) == NULL)) {
    ...
}

Could this read uninitialized or overwritten stack memory from the current
frame if a timer fires after the symbol filter string is set?

> +	memset(options, 0, sizeof(options));
> +	memset(actions, 0, sizeof(actions));
> +
>  	browser->pstack = pstack__new(3);
>  	if (browser->pstack == NULL)
>  		goto out;

[ ... ]

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

[PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop

[Ian Rogers]

In the evsel__hists_browse() interactive loop, the options array is
filled with strings dynamically allocated via asprintf() and displayed
in a popup menu. However, if the user picks a hotkey or triggers another
menu iteration, nr_options is reset to 0 without freeing the prior
strings, leaving them abandoned in the array and leaking memory on every
menu event.

Fix it by invoking free_popup_options(options, MAX_OPTIONS) at the
beginning of each menu iteration (covering both the while(1) start and
the 'do_hotkey:' label).

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 7842f228435d..70f8a820d0a0 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3087,6 +3087,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 
 		key = 0; // reset key
 do_hotkey:		 // key came straight from options ui__popup_menu()
+		free_popup_options(options, MAX_OPTIONS);
 		choice = nr_options = 0;
 		key = hist_browser__run(browser, helpline, warn_lost_event, key);
 
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop

[sashiko-bot]

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

New issues:
- [Medium] Raw pointer assignment of reference-counted structs violates REFCNT_CHECKING lifecycle rules.

Pre-existing issues:
- [High] Stack-allocated buffer `buf` is assigned to a long-lived structure field `hists->symbol_filter_str`, leading to a Use-After-Free when the function returns.
--

--- Patch [5]: [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop ---
commit 8a51dcaa31cd8e1cae7b31630b60408cb040d87b
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop

In the evsel__hists_browse() interactive loop, the options array is
filled with strings dynamically allocated via asprintf() and displayed
in a popup menu. However, if the user picks a hotkey or triggers another
menu iteration, nr_options is reset to 0 without freeing the prior
strings, leaving them abandoned in the array and leaking memory on every
menu event.

Fix it by invoking free_popup_options(options, MAX_OPTIONS) at the
beginning of each menu iteration (covering both the while(1) start and
the 'do_hotkey:' label).

Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 8b4c7510991f3..aed154622467f 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3087,6 +3087,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  
>  		key = 0; // reset key
>  do_hotkey:		 // key came straight from options ui__popup_menu()
> +		free_popup_options(options, MAX_OPTIONS);
>  		choice = nr_options = 0;
>  		key = hist_browser__run(browser, helpline, warn_lost_event, key);
>  

[ ... ]

> @@ -3156,6 +3157,6 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					if (!bi || !bi->to.ms.map)
>  						continue;
>  
>  					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
>  					actions->ms.thread = bi->to.ms.thread;
>  					actions->ms.map = bi->to.ms.map;

[Severity: Medium]
In tools/perf/ui/browsers/hists.c:evsel__hists_browse(), is it safe to
directly assign actions->ms.thread without using thread__get() when
processing the SORT_MODE__BRANCH selection?

It seems this raw pointer assignment of a reference-counted struct violates
lifecycle rules. Could this lead to asan use-after-free traps or lsan
leaks during the interactive menu loop?

[ ... ]

> @@ -3215,6 +3216,6 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  		case '/':
>  			if (ui_browser__input_window("Symbol to show",
>  					"Please enter the name of symbol you want to see.\n"
>  					"To remove the filter later, press / + ENTER.",
>  					buf, "ENTER: OK, ESC: Cancel",
>  					delay_secs * 2) == K_ENTER) {
>  				hists->symbol_filter_str = *buf ? buf : NULL;
>  				hists__filter_by_symbol(hists);

[Severity: High]
This is a pre-existing issue, but in tools/perf/ui/browsers/hists.c:evsel__hists_browse(),
doesn't this assign a stack-allocated buffer (buf) to the persistent
hists->symbol_filter_str pointer without duplicating it?

If a user applies a symbol filter in the single-event browser view and then
exits, could this cause a use-after-free when evsel__hists_browse() returns
while a background thread is still applying filters?

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

[PATCH v4 6/9] perf ui hists: Fix dso_filter reference leak and exit cleanup

[Ian Rogers]

In hists_browser__zoom_map(), the active dso is assigned as a raw
pointer to hists->dso_filter without acquiring a reference using
dso__get(). This creates a potential use-after-free defect if the
underlying dso is released while the filter remains active.

Fix it by properly acquiring a reference via dso__get() when assigning
the filter, and releasing it with dso__put() when clearing it.
Additionally, ensure that both hists->thread_filter and
hists->dso_filter are safely cleared and dropped upon exiting the
browser in evsel__hists_browse() to prevent leaking active references.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 70f8a820d0a0..316db603bf25 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2640,13 +2640,14 @@ static int hists_browser__zoom_map(struct hist_browser *browser, struct map *map
 	if (browser->hists->dso_filter) {
 		pstack__remove(browser->pstack, &browser->hists->dso_filter);
 		perf_hpp__set_elide(HISTC_DSO, false);
+		dso__put((struct dso *)browser->hists->dso_filter);
 		browser->hists->dso_filter = NULL;
 		ui_helpline__pop();
 	} else {
 		struct dso *dso = map__dso(map);
 		ui_helpline__fpush("To zoom out press ESC or ENTER + \"Zoom out of %s DSO\"",
 				   __map__is_kernel(map) ? "the Kernel" : dso__short_name(dso));
-		browser->hists->dso_filter = dso;
+		browser->hists->dso_filter = dso__get(dso);
 		perf_hpp__set_elide(HISTC_DSO, true);
 		pstack__push(browser->pstack, &browser->hists->dso_filter);
 	}
@@ -3450,6 +3451,9 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	}
 out_free_stack:
 	pstack__delete(browser->pstack);
+	thread__zput(hists->thread_filter);
+	dso__put((struct dso *)hists->dso_filter);
+	hists->dso_filter = NULL;
 out:
 	hist_browser__delete(browser);
 	free_popup_options(options, MAX_OPTIONS);
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v4 6/9] perf ui hists: Fix dso_filter reference leak and exit cleanup

[sashiko-bot]

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

New issues:
- [High] Clearing filter pointers on browser exit without reverting the corresponding filter state permanently corrupts the UI and histogram entries.

Pre-existing issues:
- [High] The `hists->symbol_filter_str` pointer is dangerously assigned to a local stack array `buf`, resulting in a dangling pointer when the function returns.
--

--- Patch [6]: [PATCH v4 6/9] perf ui hists: Fix dso_filter reference leak and exit cleanup ---
commit 817d491dc7ee2892f5d2ddca3b8801a25555bb12
Author: Ian Rogers <irogers@google.com>
Subject: perf ui hists: Fix dso_filter reference leak and exit cleanup

This commit aims to fix a potential use-after-free defect and reference
leak in the perf hists browser. It ensures dso_filter references are
properly acquired and released, and safely clears filters upon exiting
the browser.

Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> 				hists->symbol_filter_str = *buf ? buf : NULL;

[Severity: High]
This is a pre-existing issue, but might assigning the long-lived
hists->symbol_filter_str pointer directly to a local stack array (buf)
cause a use-after-return bug?

If the browser is re-entered, or in perf top where a background thread
continuously processes new samples and re-evaluates filters via
hists__filter_entry_by_symbol(), it will call strstr() on garbage
stack memory.

Could this lead to random filtering behavior, information leaks, or
crashes?

[ ... ]
> @@ -3450,6 +3451,9 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	}
>  out_free_stack:
>  	pstack__delete(browser->pstack);
> +	thread__zput(hists->thread_filter);
> +	dso__put((struct dso *)hists->dso_filter);
> +	hists->dso_filter = NULL;

[Severity: High]
Will clearing these filter pointers without reverting the corresponding
filter state leave the interface in a corrupted state?

When a user zooms into a dso or thread and exits the browser, the active
filters are nullified here but the histogram entries remain filtered
(he->filtered) and columns remain elided (perf_hpp__set_elide).

When the user re-enters the browser, they are presented with a filtered
view but have no way to 'Zoom out' because the filter pointers are now
null, replacing the 'Zoom out' menu options with 'Zoom in'.

Could this permanently break the browser functionality and corrupt the
persistent state of the hists object across browser sessions?

>  out:
>  	hist_browser__delete(browser);
>  	free_popup_options(options, MAX_OPTIONS);

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

[PATCH v4 7/9] perf ui hists: Fix NULL pointer array gap in add_script_opt()

[Ian Rogers]

In add_script_opt(), the function unconditionally increments the optstr
and act pointers for a second optional 'time' popup action before
attempting to invoke add_script_opt_2(). If the first add_script_opt_2()
call failed (for example, due to an asprintf allocation failure), this
leaves a NULL pointer gap in the options array at the prior index. When
ui__popup_menu() is later displayed, it dereferences this gap and
crashes.

Fix it by avoiding unconditional pointer increments. Only advance the
optstr and act pointers if the first script addition actually succeeded,
and safely attach the time parameter to the correct assigned action.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 316db603bf25..28f1abdd87da 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2811,7 +2811,7 @@ add_script_opt(struct hist_browser *browser,
 	       struct popup_action *act, char **optstr,
 	       struct thread *thread, struct symbol *sym)
 {
-	int n, j;
+	int n, j, ret;
 	struct hist_entry *he;
 
 	n = add_script_opt_2(act, optstr, thread, sym, "");
@@ -2819,17 +2819,24 @@ add_script_opt(struct hist_browser *browser,
 	he = hist_browser__selected_entry(browser);
 	if (sort_order && strstr(sort_order, "time")) {
 		char tstr[128];
+		struct popup_action *time_act = act;
+		char **time_optstr = optstr;
 
-		optstr++;
-		act++;
+		if (n > 0) {
+			time_optstr++;
+			time_act++;
+		}
 		j = sprintf(tstr, " in ");
 		j += timestamp__scnprintf_usec(he->time, tstr + j,
 					       sizeof tstr - j);
 		j += sprintf(tstr + j, "-");
 		timestamp__scnprintf_usec(he->time + symbol_conf.time_quantum,
 				          tstr + j, sizeof tstr - j);
-		n += add_script_opt_2(act, optstr, thread, sym, tstr);
-		act->time = he->time;
+		ret = add_script_opt_2(time_act, time_optstr, thread, sym, tstr);
+		if (ret > 0) {
+			time_act->time = he->time;
+			n += ret;
+		}
 	}
 	return n;
 }
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v4 8/9] perf ui hists: Remove duplicated thread in popup_action

[Ian Rogers]

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only ever
1 thread with a popup_action.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 28f1abdd87da..0005e454255d 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2455,7 +2455,6 @@ static int switch_data_file(void)
 
 struct popup_action {
 	unsigned long		time;
-	struct thread 		*thread;
 	int (*fn)(struct hist_browser *browser, struct popup_action *act);
 	struct map_symbol 	ms;
 	int			socket;
@@ -2572,7 +2571,7 @@ add_annotate_type_opt(struct popup_action *act, char **optstr,
 static int
 do_zoom_thread(struct hist_browser *browser, struct popup_action *act)
 {
-	struct thread *thread = act->thread;
+	struct thread *thread = act->ms.thread;
 
 	if ((!hists__has(browser->hists, thread) &&
 	     !hists__has(browser->hists, comm)) || thread == NULL)
@@ -2627,7 +2626,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
 	if (ret < 0)
 		return 0;
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->fn = do_zoom_thread;
 	return 1;
 }
@@ -2734,8 +2733,8 @@ do_run_script(struct hist_browser *browser,
 	int n = 0;
 
 	len = 100;
-	if (act->thread)
-		len += strlen(thread__comm_str(act->thread));
+	if (act->ms.thread)
+		len += strlen(thread__comm_str(act->ms.thread));
 	else if (act->ms.sym)
 		len += strlen(act->ms.sym->name);
 	script_opt = malloc(len);
@@ -2743,9 +2742,9 @@ do_run_script(struct hist_browser *browser,
 		return -1;
 
 	script_opt[0] = 0;
-	if (act->thread) {
+	if (act->ms.thread) {
 		n = scnprintf(script_opt, len, " -c %s ",
-			  thread__comm_str(act->thread));
+			  thread__comm_str(act->ms.thread));
 	} else if (act->ms.sym) {
 		n = scnprintf(script_opt, len, " -S %s ",
 			  act->ms.sym->name);
@@ -2800,7 +2799,7 @@ add_script_opt_2(struct popup_action *act, char **optstr,
 			return 0;
 	}
 
-	act->thread = thread;
+	act->ms.thread = thread;
 	act->ms.sym = sym;
 	act->fn = do_run_script;
 	return 1;
@@ -3211,7 +3210,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					   verbose);
 			continue;
 		case 't':
-			actions->thread = thread;
+			actions->ms.thread = thread;
 			do_zoom_thread(browser, actions);
 			continue;
 		case 'S':
@@ -3231,7 +3230,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 			continue;
 		case 'r':
 			if (is_report_browser(hbt)) {
-				actions->thread = NULL;
+				actions->ms.thread = NULL;
 				actions->ms.sym = NULL;
 				do_run_script(browser, actions);
 			}
@@ -3314,7 +3313,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 				 */
 				do_zoom_dso(browser, actions);
 			} else if (top == &browser->hists->thread_filter) {
-				actions->thread = thread;
+				actions->ms.thread = thread;
 				do_zoom_thread(browser, actions);
 			} else if (top == &browser->hists->socket_filter) {
 				do_zoom_socket(browser, actions);
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v4 8/9] perf ui hists: Remove duplicated thread in popup_action

[sashiko-bot]

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

Pre-existing issues:
- [Low] Missing `<limits.h>` inclusion causes a musl libc build failure due to `PATH_MAX` usage.
--

--- Patch [8]: [PATCH v4 8/9] perf ui hists: Remove duplicated thread in popup_action ---
commit cf81872962e43b7c33b75762f7881111734c277c
Author: Ian Rogers <irogers@google.com>

perf ui hists: Remove duplicated thread in popup_action

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only ever
1 thread with a popup_action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index d61ebe4f6a6a4..0713e3ddcf1f0 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2455,7 +2455,6 @@ static int switch_data_file(void)

[Severity: Low]
This isn't a bug introduced by this patch, but should limits.h be explicitly
included at the top of tools/perf/ui/browsers/hists.c?

Looking at switch_data_file(), it uses the POSIX constant PATH_MAX:

static int switch_data_file(void)
{
        char path[PATH_MAX];
        ...

Does omitting the explicit inclusion of limits.h cause build failures on musl
libc systems, which do not implicitly include headers in the same way glibc
does?

>  
>  struct popup_action {
>  	unsigned long		time;
> -	struct thread 		*thread;
>  	int (*fn)(struct hist_browser *browser, struct popup_action *act);

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

[PATCH v4 9/9] perf annotate: Be robust to annotating without a thread

[Ian Rogers]

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.

Tested running perf report without a previous fix to populate the
map_symbol thread field.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/annotate.c             |  2 +-
 .../util/annotate-arch/annotate-loongarch.c   |  4 +-
 tools/perf/util/annotate-arch/annotate-s390.c |  2 +-
 tools/perf/util/annotate.c                    | 57 ++++++++++++++-----
 tools/perf/util/annotate.h                    |  2 +-
 tools/perf/util/capstone.c                    | 30 ++++++++--
 tools/perf/util/disasm.c                      |  7 ++-
 7 files changed, 77 insertions(+), 27 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d25761a8d25e..e47a46775089 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -1201,7 +1201,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
 				ui__warning("Annotation has no source code.");
 		}
 	} else {
-		err = thread__get_arch(ms->thread, &browser.arch);
+		err = map_symbol__get_arch(ms, &browser.arch);
 		if (err) {
 			annotate_browser__symbol_annotate_error(&browser, err);
 			return -1;
diff --git a/tools/perf/util/annotate-arch/annotate-loongarch.c b/tools/perf/util/annotate-arch/annotate-loongarch.c
index c2addca77320..26217cb2d8f3 100644
--- a/tools/perf/util/annotate-arch/annotate-loongarch.c
+++ b/tools/perf/util/annotate-arch/annotate-loongarch.c
@@ -51,7 +51,7 @@ static int loongarch_call__parse(const struct arch *arch, struct ins_operands *o
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -95,7 +95,7 @@ static int loongarch_jump__parse(const struct arch *arch, struct ins_operands *o
 
 	ops->target.outside = target.addr < start || target.addr >= end;
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
diff --git a/tools/perf/util/annotate-arch/annotate-s390.c b/tools/perf/util/annotate-arch/annotate-s390.c
index af9cabd0a586..3ea6ad588998 100644
--- a/tools/perf/util/annotate-arch/annotate-s390.c
+++ b/tools/perf/util/annotate-arch/annotate-s390.c
@@ -50,7 +50,7 @@ static int s390_call__parse(const struct arch *arch, struct ins_operands *ops,
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 53b2a224b21d..cdfaeb2388d0 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)
 	annotation__calc_percent(notes, evsel, symbol__size(sym));
 }
 
-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;
+	uint32_t e_flags = 0;
+	uint16_t e_machine = EM_NONE;
 
-	if (!thread) {
-		*parch = NULL;
-		return -1;
+	/*
+	 * If we have a thread then derive the e_machine using it. The thread
+	 * can determine the machine and perf_env, so is generally pretty robust
+	 * as well as benefitting from a cache.
+	 */
+	if (ms->thread) {
+		struct maps *maps = thread__maps(ms->thread);
+
+		machine = maps__machine(maps);
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+		if (machine && machine->env)
+			cpuid = machine->env->cpuid;
+	}
+	if (e_machine == EM_NONE && ms->map) {
+		/*
+		 * If the thread look up failed then try to use a dso that may
+		 * be associated with the map.
+		 */
+		struct dso *dso = map__dso(ms->map);
+
+		if (dso)
+			e_machine = dso__e_machine(dso, machine, &e_flags);
+	}
+	if (e_machine == EM_NONE) {
+		/*
+		 * Still no e_machine, use the default for this kind of perf
+		 * build that will determine a value using uname, etc.
+		 */
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
 	}
 
-	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);
 	if (arch == NULL) {
 		pr_err("%s: unsupported arch %d\n", __func__, e_machine);
 		return errno;
@@ -1018,7 +1043,7 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
 	const struct arch *arch = NULL;
 	int err, nr;
 
-	err = thread__get_arch(ms->thread, &arch);
+	err = map_symbol__get_arch(ms, &arch);
 	if (err)
 		return err;
 
@@ -1266,7 +1291,10 @@ 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)) {
+		free(filename);
+		return -ENOTSUP;
+	}
 	apd.dbg = dso__debuginfo(dso);
 
 	list_for_each_entry(pos, &notes->src->source, node) {
@@ -1369,9 +1397,12 @@ static int symbol__annotate_fprintf2(struct symbol *sym, FILE *fp,
 		.write_graph		 = FILE__write_graph,
 	};
 	struct annotation_line *al;
+	int err;
 
 	if (annotate_opts.code_with_type) {
-		thread__get_arch(apd->he->ms.thread, &apd->arch);
+		err = map_symbol__get_arch(&apd->he->ms, &apd->arch);
+		if (err)
+			return err;
 		apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
 	}
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 1aa6df7d1618..d54f851035ec 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -584,5 +584,5 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr, u64 *br_cntr,
 			     int num_aggr, struct evsel *evsel);
 int annotation_br_cntr_abbr_list(char **str, struct evsel *evsel, bool header);
 
-int thread__get_arch(struct thread *thread, const struct arch **parch);
+int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch);
 #endif	/* __PERF_ANNOTATE_H */
diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
index 5ad537fea436..15152a2c1b25 100644
--- a/tools/perf/util/capstone.c
+++ b/tools/perf/util/capstone.c
@@ -411,9 +411,18 @@ int symbol__disassemble_capstone(const char *filename, struct symbol *sym,
 	    !strcmp(args->options->disassembler_style, "att"))
 		disassembler_style = true;
 
-	e_machine = thread__e_machine_endian(args->ms->thread,
-					     /*machine=*/NULL,
-					     /*e_flags=*/NULL, &is_big_endian);
+	if (args->ms->thread) {
+		e_machine = thread__e_machine_endian(args->ms->thread,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	} else if (dso) {
+		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
+						  &is_big_endian);
+	} else {
+		e_machine = thread__e_machine_endian(NULL,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	}
 	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
 		goto err;
 
@@ -526,9 +535,18 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
 	    !strcmp(args->options->disassembler_style, "att"))
 		disassembler_style = true;
 
-	e_machine = thread__e_machine_endian(args->ms->thread,
-					     /*machine=*/NULL,
-					     /*e_flags=*/NULL, &is_big_endian);
+	if (args->ms->thread) {
+		e_machine = thread__e_machine_endian(args->ms->thread,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	} else if (dso) {
+		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
+						  &is_big_endian);
+	} else {
+		e_machine = thread__e_machine_endian(NULL,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	}
 	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
 		goto err;
 
diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 5b114f089d56..46b9c292d5eb 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -271,7 +271,7 @@ static int call__parse(const struct arch *arch, struct ins_operands *ops, struct
 		.addr = map__objdump_2mem(map, ops->target.addr),
 	};
 
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -405,7 +405,7 @@ static int jump__parse(const struct arch *arch, struct ins_operands *ops, struct
 	 * Actual navigation will come next, with further understanding of how
 	 * the symbol searching and disassembly should be done.
 	 */
-	if (maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
+	if (ms->thread && maps__find_ams(thread__maps(ms->thread), &target) == 0 &&
 	    map__rip_2objdump(target.ms.map, map__map_ip(target.ms.map, target.addr)) == ops->target.addr)
 		ops->target.sym = target.ms.sym;
 
@@ -1068,7 +1068,8 @@ static int symbol__parse_objdump_line(struct symbol *sym,
 			.ms = { .map = map__get(map), },
 		};
 
-		if (!maps__find_ams(thread__maps(args->ms->thread), &target) &&
+		if (args->ms->thread &&
+		    !maps__find_ams(thread__maps(args->ms->thread), &target) &&
 		    target.ms.sym->start == target.al_addr)
 			dl->ops.target.sym = target.ms.sym;
 
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v4 9/9] perf annotate: Be robust to annotating without a thread

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The newly added error check for `map_symbol__get_arch` returns early and leaves malformed output on stdout.
- [Medium] Inconsistent fallback logic for `e_machine` causes capstone initialization to fail when a DSO architecture cannot be determined.
--

--- Patch [9]: [PATCH v4 9/9] perf annotate: Be robust to annotating without a thread ---
commit 0c50b4f94a148dd6a11cef4fb5824f4934eab627
Author: Ian Rogers <irogers@google.com>
Subject: 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 by falling back to reading e_machine from a map's dso or
defaulting to the host. Ensure 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..b6041f8608164 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -1266,7 +1291,10 @@ 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)) {
> +		free(filename);
> +		return -ENOTSUP;
> +	}

[Severity: Medium]
Does returning early here leave the console output in a malformed state?

Looking at hist_entry__annotate_printf(), we've already printed the
"Source code & Disassembly..." header and a dotted line to stdout before
reaching this point. Aborting here without a newline might leave the
user's console output half-printed and corrupted.

>  	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
> index 5ad537fea4360..15152a2c1b25c 100644
> --- a/tools/perf/util/capstone.c
> +++ b/tools/perf/util/capstone.c
> @@ -411,9 +411,18 @@ int symbol__disassemble_capstone(const char *filename, struct symbol *sym,
>  	    !strcmp(args->options->disassembler_style, "att"))
>  		disassembler_style = true;
>  
> -	e_machine = thread__e_machine_endian(args->ms->thread,
> -					     /*machine=*/NULL,
> -					     /*e_flags=*/NULL, &is_big_endian);
> +	if (args->ms->thread) {
> +		e_machine = thread__e_machine_endian(args->ms->thread,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	} else if (dso) {
> +		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	} else {
> +		e_machine = thread__e_machine_endian(NULL,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	}

[Severity: Medium]
If dso__e_machine_endian() returns EM_NONE, does this bypass the host
architecture fallback?

When annotating without a thread but with a valid DSO, we take this path.
If the DSO's ELF header is missing or unreadable and yields EM_NONE, the
if/else structure prevents us from executing the thread__e_machine_endian(NULL)
fallback. This could cause capstone_init() to fail and force a fallback to
the slower objdump.

>  	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
>  		goto err;
>  
> @@ -526,9 +535,18 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
>  	    !strcmp(args->options->disassembler_style, "att"))
>  		disassembler_style = true;
>  
> -	e_machine = thread__e_machine_endian(args->ms->thread,
> -					     /*machine=*/NULL,
> -					     /*e_flags=*/NULL, &is_big_endian);
> +	if (args->ms->thread) {
> +		e_machine = thread__e_machine_endian(args->ms->thread,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	} else if (dso) {
> +		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
> +						  &is_big_endian);
> +	} else {
> +		e_machine = thread__e_machine_endian(NULL,
> +						     /*machine=*/NULL,
> +						     /*e_flags=*/NULL, &is_big_endian);
> +	}

[Severity: Medium]
Does this have the same fallback logic issue as symbol__disassemble_capstone()?

If dso__e_machine_endian() returns EM_NONE, it appears we will skip the
fallback here 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/20260710024952.1668911-1-irogers@google.com?part=9

Re: [PATCH v4 1/9] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Uninitialized Pointer Free on error path in `evsel__hists_browse()`
--

--- Patch [1]: [PATCH v4 1/9] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow ---
commit e3c0cd11e9823ee7177a41443ce63fd50ac88289
Author: Ian Rogers <irogers@google.com>

perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

This commit increases MAX_OPTIONS from 16 to 32 in evsel__hists_browse() to
safely accommodate the maximum possible number of options and prevent a stack
buffer overflow.

Link: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index cfa6386e6e1da..671961946d56c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3003,7 +3003,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	struct hists *hists = evsel__hists(evsel);
>  	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
>  	struct branch_info *bi = NULL;
> -#define MAX_OPTIONS  16
> +#define MAX_OPTIONS  32
>  	char *options[MAX_OPTIONS];

[Severity: Critical]
This isn't a bug introduced by this patch, but does the error path for
pstack__new() cause an arbitrary free here?

If pstack__new(3) fails due to ENOMEM in evsel__hists_browse(), execution
jumps to the out label before the options array is zeroed by memset():

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
    char *options[MAX_OPTIONS];
    ...
    browser->pstack = pstack__new(3);
    if (browser->pstack == NULL)
        goto out;
    ...
}

The error handler then calls free_popup_options(options, MAX_OPTIONS),
which attempts to free all 32 uninitialized stack pointers.

Could a crafted perf.data file potentially prime the stack and induce an
allocation failure to exploit this arbitrary free?

>  	struct popup_action actions[MAX_OPTIONS];
>  	int nr_options = 0;

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

[PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[Ian Rogers]

In evsel__hists_browse(), the 'options' and 'actions' arrays are
statically allocated on the stack with a size of MAX_OPTIONS (16).
Further down, the function sequentially calls several add_*_opt()
functions, which increment nr_options without bounds checking.

Depending on the context (e.g., branch mode, scripting, annotations),
the sum of added options can theoretically exceed 16 (potentially
reaching up to ~19). This could lead to a stack buffer overflow.

Increase MAX_OPTIONS to 32 to safely accommodate the maximum possible
number of options without risking an overflow.

Closes: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index da7cc195b9f4..6163cc3ace27 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3003,7 +3003,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	struct hists *hists = evsel__hists(evsel);
 	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
 	struct branch_info *bi = NULL;
-#define MAX_OPTIONS  16
+#define MAX_OPTIONS  32
 	char *options[MAX_OPTIONS];
 	struct popup_action actions[MAX_OPTIONS];
 	int nr_options = 0;
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

[Ian Rogers]

Fixes heap corruption by initializing the options and actions arrays before
the pstack allocation check, preventing an uninitialized stack pointer from
being passed to free_popup_options() if the allocation fails.

Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-perf-users/20260709035230.6DBEE1F000E9@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 6163cc3ace27..319c3d6c0375 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3064,15 +3064,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 		browser->min_pcnt = min_pcnt;
 	hist_browser__update_nr_entries(browser);
 
+	memset(options, 0, sizeof(options));
+	memset(actions, 0, sizeof(actions));
+
 	browser->pstack = pstack__new(3);
 	if (browser->pstack == NULL)
 		goto out;
 
 	ui_helpline__push(helpline);
 
-	memset(options, 0, sizeof(options));
-	memset(actions, 0, sizeof(actions));
-
 	if (symbol_conf.col_width_list_str)
 		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
 
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Memory leak of dynamically allocated popup menu options during main interaction loop.
- [Medium] Unconditional pointer increments leave uninitialized or undefined array slots on allocation failure.
- [Low] Missing explicit POSIX header inclusions breaking musl libc compatibility.
--

--- Patch [2]: [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure ---
commit 77f870443b8c814782369da867fcd967bbc78bdf
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

Fixes heap corruption by initializing the options and actions arrays before
the pstack allocation check, preventing an uninitialized stack pointer from
being passed to free_popup_options() if the allocation fails.

Link: https://lore.kernel.org/linux-perf-users/20260709035230.6DBEE1F000E9@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 671961946d56c..45958bbe45092 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3064,15 +3064,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  		browser->min_pcnt = min_pcnt;
>  	hist_browser__update_nr_entries(browser);
>  
> +	memset(options, 0, sizeof(options));
> +	memset(actions, 0, sizeof(actions));
> +
>  	browser->pstack = pstack__new(3);
>  	if (browser->pstack == NULL)
>  		goto out;
>  
>  	ui_helpline__push(helpline);
>  
> -	memset(options, 0, sizeof(options));
> -	memset(actions, 0, sizeof(actions));
> -
>  	if (symbol_conf.col_width_list_str)
>  		perf_hpp__set_user_width(symbol_conf.col_width_list_str);

[Severity: Medium]
This is a pre-existing issue, but does this code leak dynamically allocated
popup menu options during the main interaction loop?

Looking at evsel__hists_browse() when returning to the do_hotkey label:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
do_hotkey:       // key came straight from options ui__popup_menu()
        choice = nr_options = 0;
    ...
}

It appears nr_options is reset to 0 without freeing the strings previously
allocated and stored in the options array. The subsequent iteration might
overwrite the string pointers, permanently leaking the previously allocated
strings. Could we ensure the strings are freed here?

[Severity: Low]
This is a pre-existing issue, but could missing explicit POSIX header
inclusions break musl libc compatibility?

For example, hist_browser__dump() uses access() and F_OK without explicitly
including <unistd.h>:

tools/perf/ui/browsers/hists.c:hist_browser__dump() {
    ...
    if (access(filename, F_OK))
    ...
}

Similarly, switch_data_file() relies on PATH_MAX without explicitly including
<limits.h>:

tools/perf/ui/browsers/hists.c:switch_data_file() {
    ...
    char path[PATH_MAX];
    ...
}

Could these headers be explicitly included to ensure musl libc builds don't
fail?

[Severity: Medium]
This is a pre-existing issue, but do unconditional pointer increments here
leave uninitialized array slots on allocation failure?

Looking at add_script_opt():

tools/perf/ui/browsers/hists.c:add_script_opt() {
    ...
        optstr++;
        act++;
    ...
}

If a previous call to add_script_opt_2() fails (e.g., asprintf fails and
returns 0), the target string pointer is left unpopulated. However, this
code unconditionally increments the array pointers. If a subsequent allocation
succeeds, the array gets populated with a gap, but nr_options is still
incremented.

Might this cause the UI menu iteration to dereference an undefined or NULL
pointer at the gap?

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

[PATCH v5 03/10] perf ui hists: Include limits.h for PATH_MAX definition

[Ian Rogers]

Looking at switch_data_file(), it uses the POSIX constant PATH_MAX.
Omitting the explicit inclusion of limits.h can cause build failures on
musl libc systems, which do not implicitly include headers in the same way
glibc does. Fix this by explicitly including <limits.h> at the top of
tools/perf/ui/browsers/hists.c.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 319c3d6c0375..9a3844dc3246 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2,6 +2,7 @@
 #include <dirent.h>
 #include <errno.h>
 #include <inttypes.h>
+#include <limits.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str

[Ian Rogers]

In evsel__hists_browse(), the local stack array 'buf' is assigned
directly to the persistent 'hists->symbol_filter_str' pointer. When the
browser returns or is exited, this dangling pointer remains active in
the 'hists' struct and is read asynchronously by the perf top background
timer, triggering a stack use-after-return.

Fix it by properly duplicating the input string using strdup(), safely
invoking zfree() to prevent memory leaks when overwriting, and cleanly
resetting and freeing the symbol filter string upon exiting the browser.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 9a3844dc3246..675910893644 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3213,7 +3213,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					"To remove the filter later, press / + ENTER.",
 					buf, "ENTER: OK, ESC: Cancel",
 					delay_secs * 2) == K_ENTER) {
-				hists->symbol_filter_str = *buf ? buf : NULL;
+				char *new_filter = *buf ? strdup(buf) : NULL;
+
+				zfree(&hists->symbol_filter_str);
+				hists->symbol_filter_str = new_filter;
 				hists__filter_by_symbol(hists);
 				hist_browser__reset(browser);
 			}
@@ -3447,6 +3450,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	}
 out_free_stack:
 	pstack__delete(browser->pstack);
+	zfree(&hists->symbol_filter_str);
+	hists__filter_by_symbol(hists);
 out:
 	hist_browser__delete(browser);
 	free_popup_options(options, MAX_OPTIONS);
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `zfree()` on `hists->symbol_filter_str` attempts to free a non-heap pointer (`argv`), causing `perf report` to crash.
- [Medium] Exiting the browser view permanently and unintentionally deletes the global command-line symbol filter.
--

--- Patch [4]: [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str ---
commit d9e40115ffaf92f2da5c8c579054c376ce729ed4
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix stack use-after-return in symbol_filter_str

The patch duplicates the input string using strdup() to prevent a stack
use-after-return, safely invoking zfree() when modifying the filter, and
resetting the filter string upon exiting the browser.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 793959574a9ad..4282ef1b81f75 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3213,7 +3213,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					"To remove the filter later, press / + ENTER.",
>  					buf, "ENTER: OK, ESC: Cancel",
>  					delay_secs * 2) == K_ENTER) {
> -				hists->symbol_filter_str = *buf ? buf : NULL;
> +				char *new_filter = *buf ? strdup(buf) : NULL;
> +
> +				zfree(&hists->symbol_filter_str);

[Severity: High]
Will calling zfree() here crash perf if the symbol filter was provided via
the command line?

When a user runs perf report with a command line filter, the argv pointer is
eventually assigned to the hists struct:

tools/perf/builtin-report.c:cmd_report() {
    ...
    report.symbol_filter_str = argv[0];
    ...
}

tools/perf/builtin-report.c:report__collapse_hists() {
    ...
    hists->symbol_filter_str = rep->symbol_filter_str;
    ...
}

Attempting to free this non-heap pointer in evsel__hists_browse() would
trigger an invalid free abort in glibc.

> +				hists->symbol_filter_str = new_filter;
>  				hists__filter_by_symbol(hists);
>  				hist_browser__reset(browser);
>  			}

[ ... ]

> @@ -3447,6 +3450,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	}
>  out_free_stack:
>  	pstack__delete(browser->pstack);
> +	zfree(&hists->symbol_filter_str);
> +	hists__filter_by_symbol(hists);

[Severity: Medium]
Does this unconditional cleanup unintentionally destroy the global
command-line filter state upon exiting the UI function?

If a user opens perf report with a command-line filter, views an event's
histogram, and then exits back to the main menu, out_free_stack clears the
filter. If they subsequently re-enter the event's histogram, the view will
be unexpectedly unfiltered.

>  out:
>  	hist_browser__delete(browser);
>  	free_popup_options(options, MAX_OPTIONS);

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

[PATCH v5 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find()

[Ian Rogers]

In arch__find(), if the architecture's arch_new_fn[e_machine]() returns
NULL, the error handling path attempts to read result->name, dereferencing
a NULL pointer and crashing. At the same time, it invokes free(tmp)
BEFORE updating the static global archs pointer to tmp. If the
reallocarray() call moved the allocated block, the original archs pointer
remained active but was freed. A subsequent call to arch__find() would
then pass this dangling pointer into bsearch(), causing a use-after-free.

Fix both by printing the numeric e_machine ID instead of result->name,
and updating the static global archs pointer to tmp immediately after the
successful reallocarray() invocation to safely retain the valid prior
architectures.

Closes: https://lore.kernel.org/linux-perf-users/20260709035721.9EE901F000E9@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/disasm.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
index 0a1a7e9cf3ef..6cfdbabbb8c7 100644
--- a/tools/perf/util/disasm.c
+++ b/tools/perf/util/disasm.c
@@ -180,14 +180,14 @@ const struct arch *arch__find(uint16_t e_machine, uint32_t e_flags, const char *
 	if (!tmp)
 		return NULL;
 
+	archs = tmp;
+
 	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);
+		pr_err("%s: failed to initialize %u arch priv area\n",
+			__func__, e_machine);
 		return NULL;
 	}
-	archs = tmp;
 	archs[num_archs++] = result;
 	qsort(archs, num_archs, sizeof(*archs), arch__cmp);
 	return result;
-- 
2.55.0.795.g602f6c329a-goog

[PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt()

[Ian Rogers]

In add_script_opt(), the function unconditionally increments the optstr
and act pointers for a second optional 'time' popup action before
attempting to invoke add_script_opt_2(). If the first add_script_opt_2()
call failed (for example, due to an asprintf allocation failure), this
leaves a NULL pointer gap in the options array at the prior index. When
ui__popup_menu() is later displayed, it dereferences this gap and
crashes.

Fix it by avoiding unconditional pointer increments. Only advance the
optstr and act pointers if the first script addition actually succeeded,
and safely attach the time parameter to the correct assigned action.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 675910893644..07a2e18f8aaf 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2811,7 +2811,7 @@ add_script_opt(struct hist_browser *browser,
 	       struct popup_action *act, char **optstr,
 	       struct thread *thread, struct symbol *sym)
 {
-	int n, j;
+	int n, j, ret;
 	struct hist_entry *he;
 
 	n = add_script_opt_2(act, optstr, thread, sym, "");
@@ -2819,17 +2819,24 @@ add_script_opt(struct hist_browser *browser,
 	he = hist_browser__selected_entry(browser);
 	if (sort_order && strstr(sort_order, "time")) {
 		char tstr[128];
+		struct popup_action *time_act = act;
+		char **time_optstr = optstr;
 
-		optstr++;
-		act++;
+		if (n > 0) {
+			time_optstr++;
+			time_act++;
+		}
 		j = sprintf(tstr, " in ");
 		j += timestamp__scnprintf_usec(he->time, tstr + j,
 					       sizeof tstr - j);
 		j += sprintf(tstr + j, "-");
 		timestamp__scnprintf_usec(he->time + symbol_conf.time_quantum,
 				          tstr + j, sizeof tstr - j);
-		n += add_script_opt_2(act, optstr, thread, sym, tstr);
-		act->time = he->time;
+		ret = add_script_opt_2(time_act, time_optstr, thread, sym, tstr);
+		if (ret > 0) {
+			time_act->time = he->time;
+			n += ret;
+		}
 	}
 	return n;
 }
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt()

[sashiko-bot]

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

Pre-existing issues:
- [Medium] Unchecked dereference of `he->time` when no histogram entry is selected and `sort_order` contains 'time'.
- [Medium] Memory leak of dynamically allocated menu option strings across iterations of the UI event loop.
--

--- Patch [6]: [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() ---
commit 6121a2411240f6a0186b5b6f8441d4151e6fa32d
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix NULL pointer array gap in add_script_opt()

This commit fixes a crash in ui__popup_menu() caused by an unconditional
pointer increment for an optional 'time' popup action. It correctly
advances the optstr and act pointers only if the first script addition
succeeds.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 4282ef1b81f75..46fb484a4dc64 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2811,7 +2811,7 @@ add_script_opt(struct hist_browser *browser,
>  	       struct popup_action *act, char **optstr,
>  	       struct thread *thread, struct symbol *sym)
>  {
> -	int n, j;
> +	int n, j, ret;
>  	struct hist_entry *he;
>  
>  	n = add_script_opt_2(act, optstr, thread, sym, "");
> @@ -2819,17 +2819,24 @@ add_script_opt(struct hist_browser *browser,
>  	he = hist_browser__selected_entry(browser);
>  	if (sort_order && strstr(sort_order, "time")) {
>  		char tstr[128];
> +		struct popup_action *time_act = act;
> +		char **time_optstr = optstr;
>  
> -		optstr++;
> -		act++;
> +		if (n > 0) {
> +			time_optstr++;
> +			time_act++;
> +		}
>  		j = sprintf(tstr, " in ");
>  		j += timestamp__scnprintf_usec(he->time, tstr + j,
>  					       sizeof tstr - j);

[Severity: Medium]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
if the histogram browser has no selected entry?

If a user interacts with an empty view and sort_order contains "time",
evsel__hists_browse() will call add_script_opt() when no entry is selected.
The call to hist_browser__selected_entry(browser) above will return NULL,
causing a crash when he->time is accessed here.

>  		j += sprintf(tstr + j, "-");
>  		timestamp__scnprintf_usec(he->time + symbol_conf.time_quantum,
>  				          tstr + j, sizeof tstr - j);
> -		n += add_script_opt_2(act, optstr, thread, sym, tstr);
> -		act->time = he->time;
> +		ret = add_script_opt_2(time_act, time_optstr, thread, sym, tstr);

[Severity: Medium]
This is a pre-existing issue, but does this leak memory when the popup menu
is repeatedly opened and closed?

The add_script_opt_2() function dynamically allocates strings into the options
array using asprintf(). In evsel__hists_browse(), the main event loop resets
the option count:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
    choice = nr_options = 0;
    ...
}

Subsequent calls to functions like add_script_opt_2() will then overwrite
these string pointers in the options array without freeing the previously
allocated strings.

> +		if (ret > 0) {
> +			time_act->time = he->time;
> +			n += ret;
> +		}
>  	}
>  	return n;
>  }

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

[PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting

[Ian Rogers]

Populates the map_symbol thread in the UI code to fix the e_machine
lookup for cross-platform disassembly when using the annotate action.
At the same time, refactor the UI options and actions generation to
comply with the perf subsystem's strict reference counting abstraction
requirements for 'struct map_symbol' and 'struct thread'. Introduce
explicit reference acquiring via map_symbol__copy() and thread__get() for
menu items, safely clean them up between menu iterations using a new
free_popup_actions() helper, and utilize insulated temporary action
variables for direct hotkey handler execution.

Assisted-by: Antigravity:gemini-3.5-flash
Reported-by: Josh Stone <jistone@redhat.com>
Fixes: 0e26ba5a8774 ("perf disasm: Refactor arch__find and initialization of arch structs")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 103 ++++++++++++++++++++++-----------
 1 file changed, 69 insertions(+), 34 deletions(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 07a2e18f8aaf..b0d421768dc5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2357,6 +2357,16 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
 	return printed;
 }
 
+struct popup_action {
+	unsigned long		time;
+	struct thread		*thread;
+	int (*fn)(struct hist_browser *browser, struct popup_action *act);
+	struct map_symbol	ms;
+	int			socket;
+	enum rstype		rstype;
+
+};
+
 static inline void free_popup_options(char **options, int n)
 {
 	int i;
@@ -2365,6 +2375,14 @@ static inline void free_popup_options(char **options, int n)
 		zfree(&options[i]);
 }
 
+static inline void free_popup_actions(struct popup_action *actions, int n)
+{
+	int i;
+
+	for (i = 0; i < n; ++i)
+		map_symbol__exit(&actions[i].ms);
+}
+
 /*
  * Only runtime switching of perf data file will make "input_name" point
  * to a malloced buffer. So add "is_input_name_malloced" flag to decide
@@ -2454,16 +2472,6 @@ static int switch_data_file(void)
 	return ret;
 }
 
-struct popup_action {
-	unsigned long		time;
-	struct thread 		*thread;
-	int (*fn)(struct hist_browser *browser, struct popup_action *act);
-	struct map_symbol 	ms;
-	int			socket;
-	enum rstype		rstype;
-
-};
-
 static int
 do_annotate(struct hist_browser *browser, struct popup_action *act)
 {
@@ -2541,7 +2549,7 @@ add_annotate_opt(struct popup_action *act, char **optstr,
 	if (asprintf(optstr, "Annotate %s", ms->sym->name) < 0)
 		return 0;
 
-	act->ms = *ms;
+	map_symbol__copy(&act->ms, ms);
 	act->fn = do_annotate;
 	return 1;
 }
@@ -2628,7 +2636,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
 	if (ret < 0)
 		return 0;
 
-	act->thread = thread;
+	act->ms.thread = thread__get(thread);
 	act->fn = do_zoom_thread;
 	return 1;
 }
@@ -2675,7 +2683,7 @@ add_dso_opt(struct hist_browser *browser, struct popup_action *act,
 		     __map__is_kernel(map) ? "the Kernel" : dso__short_name(map__dso(map))) < 0)
 		return 0;
 
-	act->ms.map = map;
+	act->ms.map = map__get(map);
 	act->fn = do_zoom_dso;
 	return 1;
 }
@@ -2720,7 +2728,7 @@ add_map_opt(struct hist_browser *browser,
 	if (asprintf(optstr, "Browse map details") < 0)
 		return 0;
 
-	act->ms.map = map;
+	act->ms.map = map__get(map);
 	act->fn = do_browse_map;
 	return 1;
 }
@@ -2800,7 +2808,7 @@ add_script_opt_2(struct popup_action *act, char **optstr,
 			return 0;
 	}
 
-	act->thread = thread;
+	act->ms.thread = thread__get(thread);
 	act->ms.sym = sym;
 	act->fn = do_run_script;
 	return 1;
@@ -3095,6 +3103,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 
 		key = 0; // reset key
 do_hotkey:		 // key came straight from options ui__popup_menu()
+		free_popup_options(options, MAX_OPTIONS);
+		free_popup_actions(actions, MAX_OPTIONS);
 		choice = nr_options = 0;
 		key = hist_browser__run(browser, helpline, warn_lost_event, key);
 
@@ -3154,24 +3164,40 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 			}
 
 			if (!browser->selection->sym) {
+				struct map_symbol source_ms;
+
 				if (!browser->he_selection)
 					continue;
 
+				memset(&source_ms, 0, sizeof(source_ms));
+
 				if (sort__mode == SORT_MODE__BRANCH) {
 					bi = browser->he_selection->branch_info;
 					if (!bi || !bi->to.ms.map)
 						continue;
 
-					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
-					actions->ms.map = bi->to.ms.map;
+					source_ms.sym =
+						symbol__new_unresolved(
+							bi->to.al_addr,
+							bi->to.ms.map);
+					source_ms.thread = bi->to.ms.thread;
+					source_ms.map = bi->to.ms.map;
 				} else {
-					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
-										 browser->selection->map);
-					actions->ms.map = browser->selection->map;
+					source_ms.sym =
+						symbol__new_unresolved(
+							browser->he_selection->ip,
+							browser->selection->map);
+					source_ms.thread = browser->selection->thread;
+					source_ms.map = browser->selection->map;
 				}
 
-				if (!actions->ms.sym)
+				if (!source_ms.sym)
 					continue;
+
+				memset(&hotkey_act, 0, sizeof(hotkey_act));
+				map_symbol__copy(&hotkey_act.ms, &source_ms);
+				do_annotate(browser, &hotkey_act);
+				map_symbol__exit(&hotkey_act.ms);
 			} else {
 				if (symbol__annotation(browser->selection->sym)->src == NULL) {
 					ui_browser__warning(&browser->b, delay_secs * 2,
@@ -3181,18 +3207,20 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					continue;
 				}
 
-				actions->ms.map = browser->selection->map;
-				actions->ms.sym = browser->selection->sym;
+				memset(&hotkey_act, 0, sizeof(hotkey_act));
+				map_symbol__copy(&hotkey_act.ms, browser->selection);
+				do_annotate(browser, &hotkey_act);
+				map_symbol__exit(&hotkey_act.ms);
 			}
-
-			do_annotate(browser, actions);
 			continue;
 		case 'P':
 			hist_browser__dump(browser);
 			continue;
 		case 'd':
-			actions->ms.map = map;
-			do_zoom_dso(browser, actions);
+			memset(&hotkey_act, 0, sizeof(hotkey_act));
+			hotkey_act.ms.map = map__get(map);
+			do_zoom_dso(browser, &hotkey_act);
+			map_symbol__exit(&hotkey_act.ms);
 			continue;
 		case 'k':
 			if (browser->selection != NULL)
@@ -3207,12 +3235,16 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 					   verbose);
 			continue;
 		case 't':
-			actions->thread = thread;
-			do_zoom_thread(browser, actions);
+			memset(&hotkey_act, 0, sizeof(hotkey_act));
+			hotkey_act.ms.thread = thread__get(thread);
+			do_zoom_thread(browser, &hotkey_act);
+			map_symbol__exit(&hotkey_act.ms);
 			continue;
 		case 'S':
-			actions->socket = socked_id;
-			do_zoom_socket(browser, actions);
+			memset(&hotkey_act, 0, sizeof(hotkey_act));
+			hotkey_act.socket = socked_id;
+			do_zoom_socket(browser, &hotkey_act);
+			map_symbol__exit(&hotkey_act.ms);
 			continue;
 		case '/':
 			if (ui_browser__input_window("Symbol to show",
@@ -3230,9 +3262,11 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 			continue;
 		case 'r':
 			if (is_report_browser(hbt)) {
-				actions->thread = NULL;
-				actions->ms.sym = NULL;
-				do_run_script(browser, actions);
+				memset(&hotkey_act, 0, sizeof(hotkey_act));
+				hotkey_act.ms.thread = NULL;
+				hotkey_act.ms.sym = NULL;
+				do_run_script(browser, &hotkey_act);
+				map_symbol__exit(&hotkey_act.ms);
 			}
 			continue;
 		case 's':
@@ -3457,6 +3491,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	}
 out_free_stack:
 	pstack__delete(browser->pstack);
+	free_popup_actions(actions, MAX_OPTIONS);
 	zfree(&hists->symbol_filter_str);
 	hists__filter_by_symbol(hists);
 out:
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The K_ESC/K_LEFT hotkey handler assigns a raw pointer to `actions->ms.map` without acquiring a reference, causing an unbalanced put and potential Use-After-Free.
- [Medium] Thread zooming and script thread filtering fail entirely and are prone to UAF due to missing updates to callback functions (`do_zoom_thread` and `do_run_script`) which still read the legacy `act->thread` field instead of `act->ms.thread`.
--

--- Patch [7]: [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting ---
commit e0c9eb4c3d452d50d71fabaeaa1ebee8659f6602
Author: Ian Rogers <irogers@google.com>

perf ui hists: In report UI ensure thread is set with reference counting

Populates the map_symbol thread in the UI code to fix the e_machine
lookup for cross-platform disassembly when using the annotate action.
At the same time, refactor the UI options and actions generation to
comply with the perf subsystem's strict reference counting abstraction
requirements for 'struct map_symbol' and 'struct thread'. Introduce
explicit reference acquiring via map_symbol__copy() and thread__get() for
menu items, safely clean them up between menu iterations using a new
free_popup_actions() helper, and utilize insulated temporary action
variables for direct hotkey handler execution.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 46fb484a4dc64..6145c7878c648 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3095,6 +3103,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  
>  		key = 0; // reset key
>  do_hotkey:		 // key came straight from options ui__popup_menu()
> +		free_popup_options(options, MAX_OPTIONS);
> +		free_popup_actions(actions, MAX_OPTIONS);
>  		choice = nr_options = 0;
>  		key = hist_browser__run(browser, helpline, warn_lost_event, key);

[Severity: Medium]
Will this lead to a refcount underflow when the user presses ESC?

If the user opens the hists browser, filters by a DSO, and presses ESC or LEFT
to zoom out, the K_ESC handler assigns the map without acquiring a reference:

evsel__hists_browse() {
    ...
    case K_ESC:
    case K_LEFT: {
        ...
        actions->ms.map = map;
        top = pstack__peek(browser->pstack);
        if (top == &browser->hists->dso_filter) {
            do_zoom_dso(browser, actions);
        }
        continue;
    }

When the loop continues and jumps back to do_hotkey, free_popup_actions()
will call map_symbol__exit() and drop a reference on actions[0].ms.map.
Since no reference was acquired with map__get(), does this underflow the map's
reference count and potentially cause a use-after-free?

[ ... ]

> @@ -2628,7 +2636,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
>  	if (ret < 0)
>  		return 0;
>  
> -	act->thread = thread;
> +	act->ms.thread = thread__get(thread);
>  	act->fn = do_zoom_thread;
>  	return 1;
>  }

[Severity: Medium]
Because act->thread is no longer set here, do the callback functions that
process this action still read the legacy act->thread field instead of
act->ms.thread?

For instance, do_zoom_thread() and do_run_script() appear to still read the
unpopulated field:

do_zoom_thread(struct hist_browser *browser, struct popup_action *act) {
    struct thread *thread = act->thread;
    ...
}

do_run_script(...) {
    if (act->thread) {
        ...
    }
}

Since the actions array is zero-initialized, act->thread will be NULL and
these functions may silently fail to apply the thread parameter.

Furthermore, if there are any remaining places in evsel__hists_browse() that
assign to the legacy actions->thread field, that field is not cleared by
free_popup_actions(). Could this leave a stale pointer in the actions array
across menu loop iterations, leading to a use-after-free if do_zoom_thread()
is subsequently executed?

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

[PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action

[Ian Rogers]

struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only ever
1 thread with a popup_action.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index b0d421768dc5..55dd1af9c81f 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2359,7 +2359,6 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
 
 struct popup_action {
 	unsigned long		time;
-	struct thread		*thread;
 	int (*fn)(struct hist_browser *browser, struct popup_action *act);
 	struct map_symbol	ms;
 	int			socket;
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unbalanced reference counting in the `K_LEFT` hotkey handler causes a premature free (UAF) of the map object.
- [Low] Removing the `thread` field from `struct popup_action` without updating remaining usages causes an immediate compilation error.
--

--- Patch [8]: [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action ---
commit 47ec26b37921590fc3d34f8d6da23b87b59a413a
Author: Ian Rogers <irogers@google.com>

perf ui hists: Remove duplicated thread in popup_action

This commit removes the thread field from struct popup_action as it is
already duplicated within the map_symbol structure. This ensures there is
only a single thread associated with a popup_action.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 6145c7878c648..89c3d3dc69a27 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2359,7 +2359,6 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
>  
>  struct popup_action {
>  	unsigned long		time;
> -	struct thread		*thread;

[Severity: Low]
Is it possible this removal causes a compilation error? There appear to be
remaining references to the thread field in this file:

tools/perf/ui/browsers/hists.c:do_zoom_thread() {
    ...
	struct thread *thread = act->thread;
    ...
}

tools/perf/ui/browsers/hists.c:do_run_script() {
    ...
		if (act->thread) {
    ...
}

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
			} else if (top == &browser->hists->thread_filter) {
				actions->thread = thread;
				do_zoom_thread(browser, actions);
    ...
}

[Severity: Medium]
Will this lead to unbalanced reference counting on the map object?

In the K_LEFT hotkey handler, an un-referenced map pointer is assigned into
the actions array before a continue statement:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
			actions->ms.map = map;
			top = pstack__peek(browser->pstack);
			if (top == &browser->hists->dso_filter) {
				/*
				 * No need to set actions->dso here since
				 * it's just to remove the current filter.
				 */
				do_zoom_dso(browser, actions);
			} else if (top == &browser->hists->thread_filter) {
				actions->thread = thread;
				do_zoom_thread(browser, actions);
			} else if (top == &browser->hists->socket_filter) {
				do_zoom_socket(browser, actions);
			}
			continue;
    ...
}

When the loop restarts at do_hotkey, free_popup_actions() drops references
for the actions array, which appears to prematurely free the map object:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
do_hotkey:		 // key came straight from options ui__popup_menu()
		free_popup_options(options, MAX_OPTIONS);
		free_popup_actions(actions, MAX_OPTIONS);
    ...
}

>  	int (*fn)(struct hist_browser *browser, struct popup_action *act);
>  	struct map_symbol	ms;
>  	int			socket;

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

[PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup

[Ian Rogers]

In hists_browser__zoom_map(), the active dso is assigned as a raw
pointer to hists->dso_filter without acquiring a reference using
dso__get(). This creates a potential use-after-free defect if the
underlying dso is released while the filter remains active.

Fix it by properly acquiring a reference via dso__get() when assigning
the filter, and releasing it with dso__put() when clearing it.
Additionally, since the browser stack relies on pstack__new() and deletes
it completely upon exiting, we must fully reset the persistent hists
structure by un-eliding columns and recalculating histogram filter
states for dso, thread, socket, and symbols upon exit to guarantee
 immaculate, uncorrupted state across browser tab switching.

Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/hists.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 55dd1af9c81f..570d1c07ca23 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2648,13 +2648,14 @@ static int hists_browser__zoom_map(struct hist_browser *browser, struct map *map
 	if (browser->hists->dso_filter) {
 		pstack__remove(browser->pstack, &browser->hists->dso_filter);
 		perf_hpp__set_elide(HISTC_DSO, false);
+		dso__put((struct dso *)browser->hists->dso_filter);
 		browser->hists->dso_filter = NULL;
 		ui_helpline__pop();
 	} else {
 		struct dso *dso = map__dso(map);
 		ui_helpline__fpush("To zoom out press ESC or ENTER + \"Zoom out of %s DSO\"",
 				   __map__is_kernel(map) ? "the Kernel" : dso__short_name(dso));
-		browser->hists->dso_filter = dso;
+		browser->hists->dso_filter = dso__get(dso);
 		perf_hpp__set_elide(HISTC_DSO, true);
 		pstack__push(browser->pstack, &browser->hists->dso_filter);
 	}
@@ -3491,6 +3492,16 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 out_free_stack:
 	pstack__delete(browser->pstack);
 	free_popup_actions(actions, MAX_OPTIONS);
+	thread__zput(hists->thread_filter);
+	dso__put((struct dso *)hists->dso_filter);
+	hists->dso_filter = NULL;
+	hists->socket_filter = -1;
+	perf_hpp__set_elide(HISTC_DSO, false);
+	perf_hpp__set_elide(HISTC_THREAD, false);
+	perf_hpp__set_elide(HISTC_SOCKET, false);
+	hists__filter_by_dso(hists);
+	hists__filter_by_thread(hists);
+	hists__filter_by_socket(hists);
 	zfree(&hists->symbol_filter_str);
 	hists__filter_by_symbol(hists);
 out:
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional free of non-heap memory causes a crash when exiting the browser or replacing a command-line symbol filter.
- [Medium] Unconditionally clearing hists filters and un-eliding columns on browser exit permanently destroys persistent command-line configurations.
--

--- Patch [9]: [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup ---
commit 1af645505e5c140a87449557e914e317c6cf9b3b
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix dso_filter reference leak and exit zoom cleanup

In hists_browser__zoom_map(), the active dso is assigned as a raw pointer
to hists->dso_filter without acquiring a reference using dso__get(). This
patch properly acquires a reference when assigning the filter, and releases
it when clearing it, while also resetting the persistent hists structure
upon exit.

Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 89c3d3dc69a27..e0ef756d95367 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c

[ ... ]

> @@ -3491,6 +3492,16 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  out_free_stack:
>  	pstack__delete(browser->pstack);
>  	free_popup_actions(actions, MAX_OPTIONS);
> +	thread__zput(hists->thread_filter);
> +	dso__put((struct dso *)hists->dso_filter);
> +	hists->dso_filter = NULL;
> +	hists->socket_filter = -1;
> +	perf_hpp__set_elide(HISTC_DSO, false);
> +	perf_hpp__set_elide(HISTC_THREAD, false);
> +	perf_hpp__set_elide(HISTC_SOCKET, false);
> +	hists__filter_by_dso(hists);
> +	hists__filter_by_thread(hists);
> +	hists__filter_by_socket(hists);

[Severity: Medium]
Could unconditionally clearing the hists filters and un-eliding columns here
permanently destroy persistent command-line configurations?

If a user launches perf report with persistent global filters like
--socket-filter 0, hists->socket_filter is initialized. When navigating into
the TUI for a specific event and then exiting back to the main menu (tab
switching), this teardown path unconditionally clears the filters (e.g.,
hists->socket_filter = -1) and un-elides the columns.

Could this destroy the user's requested command-line configuration for the
remainder of the session?

>  	zfree(&hists->symbol_filter_str);

[Severity: Medium]
Will this unconditional free cause a crash when exiting the browser or
replacing a command-line symbol filter?

When a user runs perf report --symbol-filter <sym>, the argv pointer is
assigned directly without strdup():

tools/perf/builtin-report.c:cmd_report() {
    ...
	report.symbol_filter_str = argv[0];
    ...
}

When exiting the browser, or when applying a new inline filter inside the TUI
via evsel__hists_browse(), this zfree() attempts to free the non-heap argv
string. Can this trigger a glibc abort? 

This unconditional free was introduced by commit "perf ui hists: Fix stack
use-after-return in symbol_filter_str".

>  	hists__filter_by_symbol(hists);
>  out:
>  	hist_browser__delete(browser);

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

[PATCH v5 10/10] perf annotate: Be robust to annotating without a thread

[Ian Rogers]

If a thread isn't given to map_symbol__get_arch try harder to determine
the arch for disassembly. Do this by 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
segfaults, and handle Capstone disassembly gracefully. Since
map_symbol__get_arch supports both map_symbol and thread fallbacks,
completely remove the now redundant and fragile thread__get_arch()
function and migrate all remaining callers.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/ui/browsers/annotate.c |  2 +-
 tools/perf/ui/browsers/hists.c    | 18 +++++++-------
 tools/perf/util/annotate.c        | 40 ++++++++++++++++++++-----------
 tools/perf/util/annotate.h        |  3 ++-
 tools/perf/util/capstone.c        | 36 +++++++++++++++++++++-------
 5 files changed, 67 insertions(+), 32 deletions(-)

diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
index d25761a8d25e..e47a46775089 100644
--- a/tools/perf/ui/browsers/annotate.c
+++ b/tools/perf/ui/browsers/annotate.c
@@ -1201,7 +1201,7 @@ int __hist_entry__tui_annotate(struct hist_entry *he, struct map_symbol *ms,
 				ui__warning("Annotation has no source code.");
 		}
 	} else {
-		err = thread__get_arch(ms->thread, &browser.arch);
+		err = map_symbol__get_arch(ms, &browser.arch);
 		if (err) {
 			annotate_browser__symbol_annotate_error(&browser, err);
 			return -1;
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 570d1c07ca23..7df408bde7a5 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2580,7 +2580,7 @@ add_annotate_type_opt(struct popup_action *act, char **optstr,
 static int
 do_zoom_thread(struct hist_browser *browser, struct popup_action *act)
 {
-	struct thread *thread = act->thread;
+	struct thread *thread = act->ms.thread;
 
 	if ((!hists__has(browser->hists, thread) &&
 	     !hists__has(browser->hists, comm)) || thread == NULL)
@@ -2742,8 +2742,8 @@ do_run_script(struct hist_browser *browser,
 	int n = 0;
 
 	len = 100;
-	if (act->thread)
-		len += strlen(thread__comm_str(act->thread));
+	if (act->ms.thread)
+		len += strlen(thread__comm_str(act->ms.thread));
 	else if (act->ms.sym)
 		len += strlen(act->ms.sym->name);
 	script_opt = malloc(len);
@@ -2751,9 +2751,9 @@ do_run_script(struct hist_browser *browser,
 		return -1;
 
 	script_opt[0] = 0;
-	if (act->thread) {
+	if (act->ms.thread) {
 		n = scnprintf(script_opt, len, " -c %s ",
-			  thread__comm_str(act->thread));
+			  thread__comm_str(act->ms.thread));
 	} else if (act->ms.sym) {
 		n = scnprintf(script_opt, len, " -S %s ",
 			  act->ms.sym->name);
@@ -3021,7 +3021,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 	struct branch_info *bi = NULL;
 #define MAX_OPTIONS  32
 	char *options[MAX_OPTIONS];
-	struct popup_action actions[MAX_OPTIONS];
+	struct popup_action actions[MAX_OPTIONS], hotkey_act;
 	int nr_options = 0;
 	int key = -1;
 	char buf[128];
@@ -3347,8 +3347,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
 				 */
 				do_zoom_dso(browser, actions);
 			} else if (top == &browser->hists->thread_filter) {
-				actions->thread = thread;
-				do_zoom_thread(browser, actions);
+				memset(&hotkey_act, 0, sizeof(hotkey_act));
+				hotkey_act.ms.thread = thread__get(thread);
+				do_zoom_thread(browser, &hotkey_act);
+				map_symbol__exit(&hotkey_act.ms);
 			} else if (top == &browser->hists->socket_filter) {
 				do_zoom_socket(browser, actions);
 			}
diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 53b2a224b21d..63fb09b68a12 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -982,24 +982,32 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
 	annotation__calc_percent(notes, evsel, symbol__size(sym));
 }
 
-int thread__get_arch(struct thread *thread, const struct arch **parch)
+
+
+int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch)
 {
 	const struct arch *arch;
-	struct machine *machine;
-	uint32_t e_flags;
-	uint16_t e_machine;
+	struct machine *machine = NULL;
+	struct map *map = ms->map;
+	struct dso *dso = map ? map__dso(map) : NULL;
+	uint32_t e_flags = 0;
+	uint16_t e_machine = EM_NONE;
 
-	if (!thread) {
-		*parch = NULL;
-		return -1;
+	if (ms->thread) {
+		machine = maps__machine(thread__maps(ms->thread));
+		e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+	} else if (dso) {
+		e_machine = dso__e_machine(dso, /*machine=*/NULL, &e_flags);
 	}
 
-	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);
+	if (e_machine == EM_NONE)
+		e_machine = thread__e_machine(NULL, NULL, &e_flags);
+
+	arch = arch__find(e_machine, e_flags,
+			  machine && machine->env ? machine->env->cpuid : NULL);
 	if (arch == NULL) {
 		pr_err("%s: unsupported arch %d\n", __func__, e_machine);
-		return errno;
+		return errno ? errno : -ENOTSUP;
 	}
 	if (parch)
 		*parch = arch;
@@ -1018,7 +1026,7 @@ int symbol__annotate(struct map_symbol *ms, struct evsel *evsel,
 	const struct arch *arch = NULL;
 	int err, nr;
 
-	err = thread__get_arch(ms->thread, &arch);
+	err = map_symbol__get_arch(ms, &arch);
 	if (err)
 		return err;
 
@@ -1251,6 +1259,11 @@ int hist_entry__annotate_printf(struct hist_entry *he, struct evsel *evsel)
 		evsel_name = buf;
 	}
 
+	if (map_symbol__get_arch(ms, &apd.arch)) {
+		free(filename);
+		return -ENOTSUP;
+	}
+
 	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " samples, "
 				  "percent: %s)\n",
 				  width, width, symbol_conf.show_total_period ? "Period" :
@@ -1266,7 +1279,6 @@ 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);
 	apd.dbg = dso__debuginfo(dso);
 
 	list_for_each_entry(pos, &notes->src->source, node) {
@@ -1371,7 +1383,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);
 		apd->dbg = dso__debuginfo(map__dso(apd->he->ms.map));
 	}
 
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 1aa6df7d1618..fa08d09b80f7 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -584,5 +584,6 @@ int annotation_br_cntr_entry(char **str, int br_cntr_nr, u64 *br_cntr,
 			     int num_aggr, struct evsel *evsel);
 int annotation_br_cntr_abbr_list(char **str, struct evsel *evsel, bool header);
 
-int thread__get_arch(struct thread *thread, const struct arch **parch);
+
+int map_symbol__get_arch(struct map_symbol *ms, const struct arch **parch);
 #endif	/* __PERF_ANNOTATE_H */
diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
index 5ad537fea436..bbb68d66466f 100644
--- a/tools/perf/util/capstone.c
+++ b/tools/perf/util/capstone.c
@@ -380,7 +380,7 @@ int symbol__disassemble_capstone(const char *filename, struct symbol *sym,
 	char disasm_buf[512];
 	struct disasm_line *dl;
 	bool disassembler_style = false;
-	uint16_t e_machine;
+	uint16_t e_machine = EM_NONE;
 	bool is_big_endian = false;
 
 	if (args->options->objdump_path)
@@ -411,9 +411,19 @@ int symbol__disassemble_capstone(const char *filename, struct symbol *sym,
 	    !strcmp(args->options->disassembler_style, "att"))
 		disassembler_style = true;
 
-	e_machine = thread__e_machine_endian(args->ms->thread,
-					     /*machine=*/NULL,
-					     /*e_flags=*/NULL, &is_big_endian);
+	if (args->ms->thread) {
+		e_machine = thread__e_machine_endian(args->ms->thread,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	} else if (dso) {
+		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
+						  &is_big_endian);
+	}
+	if (!e_machine || e_machine == EM_NONE) {
+		e_machine = thread__e_machine_endian(NULL,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	}
 	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
 		goto err;
 
@@ -506,7 +516,7 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
 	struct disasm_line *dl;
 	u32 *line;
 	bool disassembler_style = false;
-	uint16_t e_machine;
+	uint16_t e_machine = EM_NONE;
 	bool is_big_endian = false;
 
 	if (args->options->objdump_path)
@@ -526,9 +536,19 @@ int symbol__disassemble_capstone_powerpc(const char *filename __maybe_unused,
 	    !strcmp(args->options->disassembler_style, "att"))
 		disassembler_style = true;
 
-	e_machine = thread__e_machine_endian(args->ms->thread,
-					     /*machine=*/NULL,
-					     /*e_flags=*/NULL, &is_big_endian);
+	if (args->ms->thread) {
+		e_machine = thread__e_machine_endian(args->ms->thread,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	} else if (dso) {
+		e_machine = dso__e_machine_endian(dso, /*machine=*/NULL, /*e_flags=*/NULL,
+						  &is_big_endian);
+	}
+	if (!e_machine || e_machine == EM_NONE) {
+		e_machine = thread__e_machine_endian(NULL,
+						     /*machine=*/NULL,
+						     /*e_flags=*/NULL, &is_big_endian);
+	}
 	if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
 		goto err;
 
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Double-put of the `map` struct leading to a use-after-free when zooming out via the K_ESC/K_LEFT hotkeys.
--

--- Patch [10]: [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread ---
commit 380963af7e8bad2a7eee6ec6e6e6656a899a7aa7
Author: Ian Rogers <irogers@google.com>

perf annotate: Be robust to annotating without a thread

This commit makes perf annotate more robust by trying harder to determine
the architecture for disassembly when a thread isn't given. It introduces
map_symbol__get_arch() to replace the now removed thread__get_arch() and
safely handles map_symbol fallbacks.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index e0ef756d95367..aee3d2c4e1867 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -3341,14 +3341,16 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  			actions->ms.map = map;

[Severity: Medium]
Could this assignment to actions->ms.map lead to a use-after-free?

When zooming out via the K_ESC or K_LEFT hotkeys, actions->ms.map is assigned
the current map pointer without acquiring a reference via map__get().

The loop then continues to the top, where free_popup_actions() is called on
the actions array. 

Does free_popup_actions() systematically call map_symbol__exit(), causing it
to improperly drop a reference on actions[0].ms.map? This could lead to a
reference count underflow and use-after-free of the map.

>  			top = pstack__peek(browser->pstack);
>  			if (top == &browser->hists->dso_filter) {
>  				/*
>  				 * No need to set actions->dso here since
>  				 * it's just to remove the current filter.
>  				 */
>  				do_zoom_dso(browser, actions);
>  			} else if (top == &browser->hists->thread_filter) {
> -				actions->thread = thread;
> -				do_zoom_thread(browser, actions);
> +				memset(&hotkey_act, 0, sizeof(hotkey_act));
> +				hotkey_act.ms.thread = thread__get(thread);
> +				do_zoom_thread(browser, &hotkey_act);
> +				map_symbol__exit(&hotkey_act.ms);
>  			} else if (top == &browser->hists->socket_filter) {
>  				do_zoom_socket(browser, actions);
>  			}

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

Re: [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[sashiko-bot]

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

Pre-existing issues:
- [High] A pointer to a local stack array is assigned to a longer-lived structure field (`hists->symbol_filter_str`), leading to a Use-After-Free.
--

--- Patch [1]: [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow ---
commit b1c79545734c108ba31495cc1454bc12dd7ce4ad
Author: Ian Rogers <irogers@google.com>

perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

In evsel__hists_browse(), the options and actions arrays are statically
allocated on the stack with a size of MAX_OPTIONS. Depending on the context,
the sum of added options can theoretically exceed 16, potentially leading to
a stack buffer overflow. This patch increases MAX_OPTIONS to 32 to safely
accommodate the maximum possible number of options.

Link: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index cfa6386e6e1da..671961946d56c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3003,7 +3003,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	struct hists *hists = evsel__hists(evsel);
>  	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
>  	struct branch_info *bi = NULL;
> -#define MAX_OPTIONS  16
> +#define MAX_OPTIONS  32
>  	char *options[MAX_OPTIONS];
>  	struct popup_action actions[MAX_OPTIONS];
>  	int nr_options = 0;

[Severity: High]
This isn't a bug introduced by this patch, but since we are looking at
evsel__hists_browse(), can this cause a use-after-free later?

Just below this context, there is a local stack array declared:

    char buf[128];

Later in the function, when handling the '/' key for symbol filtering,
a pointer to this local stack array is assigned to a persistent field:

    case '/':
        if (ui_browser__input_window("Symbol to show",
                ...
                buf, "ENTER: OK, ESC: Cancel",
                delay_secs * 2) == K_ENTER) {
            hists->symbol_filter_str = *buf ? buf : NULL;
            ...

When the function returns and the user exits the browser, buf is destroyed,
but hists->symbol_filter_str will still point to this invalid stack memory.

In a multi-threaded context like perf top, if background processing threads or
subsequent timer-driven UI refreshes (via hists__filter_by_symbol() ->
hists__filter_entry_by_symbol() -> strstr()) read from it:

    strstr(he->ms.sym->name, hists->symbol_filter_str)

Could this lead to memory corruption or crashes?

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