* [PATCH v1 1/3] perf ui hists: In report UI ensure thread is set
@ 2026-07-08 23:46 Ian Rogers
2026-07-08 23:46 ` [PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
` (2 more replies)
0 siblings, 3 replies; 30+ messages in thread
From: Ian Rogers @ 2026-07-08 23:46 UTC (permalink / raw)
To: Josh Stone, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
Namhyung Kim, Jiri Olsa, Ian Rogers, Adrian Hunter, James Clark,
Tianyou Li, Dapeng Mi, Zecheng Li, linux-perf-users, linux-kernel
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
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action 2026-07-08 23:46 [PATCH v1 1/3] perf ui hists: In report UI ensure thread is set Ian Rogers @ 2026-07-08 23:46 ` Ian Rogers 2026-07-08 23:46 ` [PATCH v1 3/3] perf annotate: Be robust to annotating without a thread Ian Rogers 2026-07-09 3:36 ` [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-08 23:46 UTC (permalink / raw) To: Josh Stone, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Jiri Olsa, Ian Rogers, Adrian Hunter, James Clark, Tianyou Li, Dapeng Mi, Zecheng Li, linux-perf-users, linux-kernel 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v1 3/3] perf annotate: Be robust to annotating without a thread 2026-07-08 23:46 [PATCH v1 1/3] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-08 23:46 ` [PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action Ian Rogers @ 2026-07-08 23:46 ` Ian Rogers 2026-07-09 3:36 ` [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-08 23:46 UTC (permalink / raw) To: Josh Stone, Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Jiri Olsa, Ian Rogers, Adrian Hunter, James Clark, Tianyou Li, Dapeng Mi, Zecheng Li, linux-perf-users, linux-kernel 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(¬es->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, ¬es->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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set 2026-07-08 23:46 [PATCH v1 1/3] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-08 23:46 ` [PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-08 23:46 ` [PATCH v1 3/3] perf annotate: Be robust to annotating without a thread Ian Rogers @ 2026-07-09 3:36 ` Ian Rogers 2026-07-09 3:36 ` [PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers ` (3 more replies) 2 siblings, 4 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 3:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action 2026-07-09 3:36 ` [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers @ 2026-07-09 3:36 ` Ian Rogers 2026-07-09 3:37 ` [PATCH v2 3/4] perf annotate: Be robust to annotating without a thread Ian Rogers ` (2 subsequent siblings) 3 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 3:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 3/4] perf annotate: Be robust to annotating without a thread 2026-07-09 3:36 ` [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-09 3:36 ` [PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers @ 2026-07-09 3:37 ` Ian Rogers 2026-07-09 3:37 ` [PATCH v2 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 2026-07-09 16:52 ` [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 3 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 3:37 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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(¬es->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, ¬es->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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v2 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow 2026-07-09 3:36 ` [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-09 3:36 ` [PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-09 3:37 ` [PATCH v2 3/4] perf annotate: Be robust to annotating without a thread Ian Rogers @ 2026-07-09 3:37 ` Ian Rogers 2026-07-09 16:52 ` [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 3 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 3:37 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set 2026-07-09 3:36 ` [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers ` (2 preceding siblings ...) 2026-07-09 3:37 ` [PATCH v2 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers @ 2026-07-09 16:52 ` Ian Rogers 2026-07-09 16:52 ` [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers ` (3 more replies) 3 siblings, 4 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 16:52 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action 2026-07-09 16:52 ` [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers @ 2026-07-09 16:52 ` Ian Rogers 2026-07-09 16:52 ` [PATCH v3 3/4] perf annotate: Be robust to annotating without a thread Ian Rogers ` (2 subsequent siblings) 3 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 16:52 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 3/4] perf annotate: Be robust to annotating without a thread 2026-07-09 16:52 ` [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-09 16:52 ` [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers @ 2026-07-09 16:52 ` Ian Rogers 2026-07-09 16:52 ` [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers 3 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 16:52 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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(¬es->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, ¬es->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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow 2026-07-09 16:52 ` [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-09 16:52 ` [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-09 16:52 ` [PATCH v3 3/4] perf annotate: Be robust to annotating without a thread Ian Rogers @ 2026-07-09 16:52 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers 3 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-09 16:52 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 1/9] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow 2026-07-09 16:52 ` [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers ` (2 preceding siblings ...) 2026-07-09 16:52 ` [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 2/9] perf ui hists: In report UI ensure thread is set Ian Rogers ` (8 more replies) 3 siblings, 9 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 2/9] perf ui hists: In report UI ensure thread is set 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 3/9] perf disasm: Fix potential NULL pointer dereference in arch__find() Ian Rogers ` (7 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 3/9] perf disasm: Fix potential NULL pointer dereference in arch__find() 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers 2026-07-10 2:49 ` [PATCH v4 2/9] perf ui hists: In report UI ensure thread is set Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 4/9] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers ` (6 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 4/9] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers 2026-07-10 2:49 ` [PATCH v4 2/9] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-10 2:49 ` [PATCH v4 3/9] perf disasm: Fix potential NULL pointer dereference in arch__find() Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop Ian Rogers ` (5 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers ` (2 preceding siblings ...) 2026-07-10 2:49 ` [PATCH v4 4/9] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 6/9] perf ui hists: Fix dso_filter reference leak and exit cleanup Ian Rogers ` (4 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 6/9] perf ui hists: Fix dso_filter reference leak and exit cleanup 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers ` (3 preceding siblings ...) 2026-07-10 2:49 ` [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 7/9] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers ` (3 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 7/9] perf ui hists: Fix NULL pointer array gap in add_script_opt() 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers ` (4 preceding siblings ...) 2026-07-10 2:49 ` [PATCH v4 6/9] perf ui hists: Fix dso_filter reference leak and exit cleanup Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 8/9] perf ui hists: Remove duplicated thread in popup_action Ian Rogers ` (2 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 8/9] perf ui hists: Remove duplicated thread in popup_action 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers ` (5 preceding siblings ...) 2026-07-10 2:49 ` [PATCH v4 7/9] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 2:49 ` [PATCH v4 9/9] perf annotate: Be robust to annotating without a thread Ian Rogers 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v4 9/9] perf annotate: Be robust to annotating without a thread 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers ` (6 preceding siblings ...) 2026-07-10 2:49 ` [PATCH v4 8/9] perf ui hists: Remove duplicated thread in popup_action Ian Rogers @ 2026-07-10 2:49 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 2:49 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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(¬es->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, ¬es->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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers ` (7 preceding siblings ...) 2026-07-10 2:49 ` [PATCH v4 9/9] perf annotate: Be robust to annotating without a thread Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers ` (8 more replies) 8 siblings, 9 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 03/10] perf ui hists: Include limits.h for PATH_MAX definition Ian Rogers ` (7 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li, sashiko-bot 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 03/10] perf ui hists: Include limits.h for PATH_MAX definition 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 2026-07-10 5:36 ` [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers ` (6 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 2026-07-10 5:36 ` [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers 2026-07-10 5:36 ` [PATCH v5 03/10] perf ui hists: Include limits.h for PATH_MAX definition Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() Ian Rogers ` (5 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers ` (2 preceding siblings ...) 2026-07-10 5:36 ` [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers ` (4 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers ` (3 preceding siblings ...) 2026-07-10 5:36 ` [PATCH v5 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers ` (3 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers ` (4 preceding siblings ...) 2026-07-10 5:36 ` [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers ` (2 subsequent siblings) 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers ` (5 preceding siblings ...) 2026-07-10 5:36 ` [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers 2026-07-10 5:36 ` [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers ` (6 preceding siblings ...) 2026-07-10 5:36 ` [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 2026-07-10 5:36 ` [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
* [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers ` (7 preceding siblings ...) 2026-07-10 5:36 ` [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers @ 2026-07-10 5:36 ` Ian Rogers 8 siblings, 0 replies; 30+ messages in thread From: Ian Rogers @ 2026-07-10 5:36 UTC (permalink / raw) To: irogers, acme, jistone, namhyung Cc: adrian.hunter, dapeng1.mi, james.clark, jolsa, linux-kernel, linux-perf-users, mingo, peterz, tianyou.li 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(¬es->src->source, notes->src->start); - thread__get_arch(ms->thread, &apd.arch); apd.dbg = dso__debuginfo(dso); list_for_each_entry(pos, ¬es->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 ^ permalink raw reply related [flat|nested] 30+ messages in thread
end of thread, other threads:[~2026-07-10 5:36 UTC | newest] Thread overview: 30+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-07-08 23:46 [PATCH v1 1/3] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-08 23:46 ` [PATCH v1 2/3] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-08 23:46 ` [PATCH v1 3/3] perf annotate: Be robust to annotating without a thread Ian Rogers 2026-07-09 3:36 ` [PATCH v2 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-09 3:36 ` [PATCH v2 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-09 3:37 ` [PATCH v2 3/4] perf annotate: Be robust to annotating without a thread Ian Rogers 2026-07-09 3:37 ` [PATCH v2 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 2026-07-09 16:52 ` [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-09 16:52 ` [PATCH v3 2/4] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-09 16:52 ` [PATCH v3 3/4] perf annotate: Be robust to annotating without a thread Ian Rogers 2026-07-09 16:52 ` [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 2026-07-10 2:49 ` [PATCH v4 1/9] " Ian Rogers 2026-07-10 2:49 ` [PATCH v4 2/9] perf ui hists: In report UI ensure thread is set Ian Rogers 2026-07-10 2:49 ` [PATCH v4 3/9] perf disasm: Fix potential NULL pointer dereference in arch__find() Ian Rogers 2026-07-10 2:49 ` [PATCH v4 4/9] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers 2026-07-10 2:49 ` [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop Ian Rogers 2026-07-10 2:49 ` [PATCH v4 6/9] perf ui hists: Fix dso_filter reference leak and exit cleanup Ian Rogers 2026-07-10 2:49 ` [PATCH v4 7/9] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers 2026-07-10 2:49 ` [PATCH v4 8/9] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-10 2:49 ` [PATCH v4 9/9] perf annotate: Be robust to annotating without a thread Ian Rogers 2026-07-10 5:36 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Ian Rogers 2026-07-10 5:36 ` [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers 2026-07-10 5:36 ` [PATCH v5 03/10] perf ui hists: Include limits.h for PATH_MAX definition Ian Rogers 2026-07-10 5:36 ` [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers 2026-07-10 5:36 ` [PATCH v5 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() Ian Rogers 2026-07-10 5:36 ` [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers 2026-07-10 5:36 ` [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers 2026-07-10 5:36 ` [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers 2026-07-10 5:36 ` [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers 2026-07-10 5:36 ` [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox