* [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] " Ian Rogers
@ 2026-07-10 5:36 ` Ian Rogers
2026-07-10 5:55 ` sashiko-bot
2026-07-10 5:36 ` [PATCH v5 03/10] perf ui hists: Include limits.h for PATH_MAX definition Ian Rogers
` (9 subsequent siblings)
10 siblings, 1 reply; 89+ 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] 89+ messages in thread* Re: [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure
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:55 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 5:55 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Memory leak of dynamically allocated popup menu options during main interaction loop.
- [Medium] Unconditional pointer increments leave uninitialized or undefined array slots on allocation failure.
- [Low] Missing explicit POSIX header inclusions breaking musl libc compatibility.
--
--- Patch [2]: [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure ---
commit 77f870443b8c814782369da867fcd967bbc78bdf
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix uninitialized stack memory free on pstack allocation failure
Fixes heap corruption by initializing the options and actions arrays before
the pstack allocation check, preventing an uninitialized stack pointer from
being passed to free_popup_options() if the allocation fails.
Link: https://lore.kernel.org/linux-perf-users/20260709035230.6DBEE1F000E9@smtp.kernel.org/
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 671961946d56c..45958bbe45092 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3064,15 +3064,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> browser->min_pcnt = min_pcnt;
> hist_browser__update_nr_entries(browser);
>
> + memset(options, 0, sizeof(options));
> + memset(actions, 0, sizeof(actions));
> +
> browser->pstack = pstack__new(3);
> if (browser->pstack == NULL)
> goto out;
>
> ui_helpline__push(helpline);
>
> - memset(options, 0, sizeof(options));
> - memset(actions, 0, sizeof(actions));
> -
> if (symbol_conf.col_width_list_str)
> perf_hpp__set_user_width(symbol_conf.col_width_list_str);
[Severity: Medium]
This is a pre-existing issue, but does this code leak dynamically allocated
popup menu options during the main interaction loop?
Looking at evsel__hists_browse() when returning to the do_hotkey label:
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
do_hotkey: // key came straight from options ui__popup_menu()
choice = nr_options = 0;
...
}
It appears nr_options is reset to 0 without freeing the strings previously
allocated and stored in the options array. The subsequent iteration might
overwrite the string pointers, permanently leaking the previously allocated
strings. Could we ensure the strings are freed here?
[Severity: Low]
This is a pre-existing issue, but could missing explicit POSIX header
inclusions break musl libc compatibility?
For example, hist_browser__dump() uses access() and F_OK without explicitly
including <unistd.h>:
tools/perf/ui/browsers/hists.c:hist_browser__dump() {
...
if (access(filename, F_OK))
...
}
Similarly, switch_data_file() relies on PATH_MAX without explicitly including
<limits.h>:
tools/perf/ui/browsers/hists.c:switch_data_file() {
...
char path[PATH_MAX];
...
}
Could these headers be explicitly included to ensure musl libc builds don't
fail?
[Severity: Medium]
This is a pre-existing issue, but do unconditional pointer increments here
leave uninitialized array slots on allocation failure?
Looking at add_script_opt():
tools/perf/ui/browsers/hists.c:add_script_opt() {
...
optstr++;
act++;
...
}
If a previous call to add_script_opt_2() fails (e.g., asprintf fails and
returns 0), the target string pointer is left unpopulated. However, this
code unconditionally increments the array pointers. If a subsequent allocation
succeeds, the array gets populated with a gap, but nr_options is still
incremented.
Might this cause the UI menu iteration to dereference an undefined or NULL
pointer at the gap?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=2
^ permalink raw reply [flat|nested] 89+ 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] " 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
` (8 subsequent siblings)
10 siblings, 0 replies; 89+ 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] 89+ 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] " 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:59 ` sashiko-bot
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
` (7 subsequent siblings)
10 siblings, 1 reply; 89+ 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] 89+ messages in thread* Re: [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str
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:59 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 5:59 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `zfree()` on `hists->symbol_filter_str` attempts to free a non-heap pointer (`argv`), causing `perf report` to crash.
- [Medium] Exiting the browser view permanently and unintentionally deletes the global command-line symbol filter.
--
--- Patch [4]: [PATCH v5 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str ---
commit d9e40115ffaf92f2da5c8c579054c376ce729ed4
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix stack use-after-return in symbol_filter_str
The patch duplicates the input string using strdup() to prevent a stack
use-after-return, safely invoking zfree() when modifying the filter, and
resetting the filter string upon exiting the browser.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 793959574a9ad..4282ef1b81f75 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3213,7 +3213,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> "To remove the filter later, press / + ENTER.",
> buf, "ENTER: OK, ESC: Cancel",
> delay_secs * 2) == K_ENTER) {
> - hists->symbol_filter_str = *buf ? buf : NULL;
> + char *new_filter = *buf ? strdup(buf) : NULL;
> +
> + zfree(&hists->symbol_filter_str);
[Severity: High]
Will calling zfree() here crash perf if the symbol filter was provided via
the command line?
When a user runs perf report with a command line filter, the argv pointer is
eventually assigned to the hists struct:
tools/perf/builtin-report.c:cmd_report() {
...
report.symbol_filter_str = argv[0];
...
}
tools/perf/builtin-report.c:report__collapse_hists() {
...
hists->symbol_filter_str = rep->symbol_filter_str;
...
}
Attempting to free this non-heap pointer in evsel__hists_browse() would
trigger an invalid free abort in glibc.
> + hists->symbol_filter_str = new_filter;
> hists__filter_by_symbol(hists);
> hist_browser__reset(browser);
> }
[ ... ]
> @@ -3447,6 +3450,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> }
> out_free_stack:
> pstack__delete(browser->pstack);
> + zfree(&hists->symbol_filter_str);
> + hists__filter_by_symbol(hists);
[Severity: Medium]
Does this unconditional cleanup unintentionally destroy the global
command-line filter state upon exiting the UI function?
If a user opens perf report with a command-line filter, views an event's
histogram, and then exits back to the main menu, out_free_stack clears the
filter. If they subsequently re-enter the event's histogram, the view will
be unexpectedly unfiltered.
> out:
> hist_browser__delete(browser);
> free_popup_options(options, MAX_OPTIONS);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=4
^ permalink raw reply [flat|nested] 89+ 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] " 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
` (6 subsequent siblings)
10 siblings, 0 replies; 89+ 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] 89+ 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] " 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:56 ` sashiko-bot
2026-07-10 5:36 ` [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers
` (5 subsequent siblings)
10 siblings, 1 reply; 89+ 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] 89+ messages in thread* Re: [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt()
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:56 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 5:56 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Unchecked dereference of `he->time` when no histogram entry is selected and `sort_order` contains 'time'.
- [Medium] Memory leak of dynamically allocated menu option strings across iterations of the UI event loop.
--
--- Patch [6]: [PATCH v5 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() ---
commit 6121a2411240f6a0186b5b6f8441d4151e6fa32d
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix NULL pointer array gap in add_script_opt()
This commit fixes a crash in ui__popup_menu() caused by an unconditional
pointer increment for an optional 'time' popup action. It correctly
advances the optstr and act pointers only if the first script addition
succeeds.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 4282ef1b81f75..46fb484a4dc64 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2811,7 +2811,7 @@ add_script_opt(struct hist_browser *browser,
> struct popup_action *act, char **optstr,
> struct thread *thread, struct symbol *sym)
> {
> - int n, j;
> + int n, j, ret;
> struct hist_entry *he;
>
> n = add_script_opt_2(act, optstr, thread, sym, "");
> @@ -2819,17 +2819,24 @@ add_script_opt(struct hist_browser *browser,
> he = hist_browser__selected_entry(browser);
> if (sort_order && strstr(sort_order, "time")) {
> char tstr[128];
> + struct popup_action *time_act = act;
> + char **time_optstr = optstr;
>
> - optstr++;
> - act++;
> + if (n > 0) {
> + time_optstr++;
> + time_act++;
> + }
> j = sprintf(tstr, " in ");
> j += timestamp__scnprintf_usec(he->time, tstr + j,
> sizeof tstr - j);
[Severity: Medium]
This is a pre-existing issue, but can this lead to a NULL pointer dereference
if the histogram browser has no selected entry?
If a user interacts with an empty view and sort_order contains "time",
evsel__hists_browse() will call add_script_opt() when no entry is selected.
The call to hist_browser__selected_entry(browser) above will return NULL,
causing a crash when he->time is accessed here.
> j += sprintf(tstr + j, "-");
> timestamp__scnprintf_usec(he->time + symbol_conf.time_quantum,
> tstr + j, sizeof tstr - j);
> - n += add_script_opt_2(act, optstr, thread, sym, tstr);
> - act->time = he->time;
> + ret = add_script_opt_2(time_act, time_optstr, thread, sym, tstr);
[Severity: Medium]
This is a pre-existing issue, but does this leak memory when the popup menu
is repeatedly opened and closed?
The add_script_opt_2() function dynamically allocates strings into the options
array using asprintf(). In evsel__hists_browse(), the main event loop resets
the option count:
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
choice = nr_options = 0;
...
}
Subsequent calls to functions like add_script_opt_2() will then overwrite
these string pointers in the options array without freeing the previously
allocated strings.
> + if (ret > 0) {
> + time_act->time = he->time;
> + n += ret;
> + }
> }
> return n;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=6
^ permalink raw reply [flat|nested] 89+ 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] " 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:54 ` sashiko-bot
2026-07-10 5:36 ` [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
` (4 subsequent siblings)
10 siblings, 1 reply; 89+ 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] 89+ messages in thread* Re: [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting
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:54 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 5:54 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The K_ESC/K_LEFT hotkey handler assigns a raw pointer to `actions->ms.map` without acquiring a reference, causing an unbalanced put and potential Use-After-Free.
- [Medium] Thread zooming and script thread filtering fail entirely and are prone to UAF due to missing updates to callback functions (`do_zoom_thread` and `do_run_script`) which still read the legacy `act->thread` field instead of `act->ms.thread`.
--
--- Patch [7]: [PATCH v5 07/10] perf ui hists: In report UI ensure thread is set with reference counting ---
commit e0c9eb4c3d452d50d71fabaeaa1ebee8659f6602
Author: Ian Rogers <irogers@google.com>
perf ui hists: In report UI ensure thread is set with reference counting
Populates the map_symbol thread in the UI code to fix the e_machine
lookup for cross-platform disassembly when using the annotate action.
At the same time, refactor the UI options and actions generation to
comply with the perf subsystem's strict reference counting abstraction
requirements for 'struct map_symbol' and 'struct thread'. Introduce
explicit reference acquiring via map_symbol__copy() and thread__get() for
menu items, safely clean them up between menu iterations using a new
free_popup_actions() helper, and utilize insulated temporary action
variables for direct hotkey handler execution.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 46fb484a4dc64..6145c7878c648 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3095,6 +3103,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>
> key = 0; // reset key
> do_hotkey: // key came straight from options ui__popup_menu()
> + free_popup_options(options, MAX_OPTIONS);
> + free_popup_actions(actions, MAX_OPTIONS);
> choice = nr_options = 0;
> key = hist_browser__run(browser, helpline, warn_lost_event, key);
[Severity: Medium]
Will this lead to a refcount underflow when the user presses ESC?
If the user opens the hists browser, filters by a DSO, and presses ESC or LEFT
to zoom out, the K_ESC handler assigns the map without acquiring a reference:
evsel__hists_browse() {
...
case K_ESC:
case K_LEFT: {
...
actions->ms.map = map;
top = pstack__peek(browser->pstack);
if (top == &browser->hists->dso_filter) {
do_zoom_dso(browser, actions);
}
continue;
}
When the loop continues and jumps back to do_hotkey, free_popup_actions()
will call map_symbol__exit() and drop a reference on actions[0].ms.map.
Since no reference was acquired with map__get(), does this underflow the map's
reference count and potentially cause a use-after-free?
[ ... ]
> @@ -2628,7 +2636,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
> if (ret < 0)
> return 0;
>
> - act->thread = thread;
> + act->ms.thread = thread__get(thread);
> act->fn = do_zoom_thread;
> return 1;
> }
[Severity: Medium]
Because act->thread is no longer set here, do the callback functions that
process this action still read the legacy act->thread field instead of
act->ms.thread?
For instance, do_zoom_thread() and do_run_script() appear to still read the
unpopulated field:
do_zoom_thread(struct hist_browser *browser, struct popup_action *act) {
struct thread *thread = act->thread;
...
}
do_run_script(...) {
if (act->thread) {
...
}
}
Since the actions array is zero-initialized, act->thread will be NULL and
these functions may silently fail to apply the thread parameter.
Furthermore, if there are any remaining places in evsel__hists_browse() that
assign to the legacy actions->thread field, that field is not cleared by
free_popup_actions(). Could this leave a stale pointer in the actions array
across menu loop iterations, leading to a use-after-free if do_zoom_thread()
is subsequently executed?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=7
^ permalink raw reply [flat|nested] 89+ 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] " 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:54 ` sashiko-bot
2026-07-10 5:36 ` [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers
` (3 subsequent siblings)
10 siblings, 1 reply; 89+ 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] 89+ messages in thread* Re: [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action
2026-07-10 5:36 ` [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
@ 2026-07-10 5:54 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 5:54 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unbalanced reference counting in the `K_LEFT` hotkey handler causes a premature free (UAF) of the map object.
- [Low] Removing the `thread` field from `struct popup_action` without updating remaining usages causes an immediate compilation error.
--
--- Patch [8]: [PATCH v5 08/10] perf ui hists: Remove duplicated thread in popup_action ---
commit 47ec26b37921590fc3d34f8d6da23b87b59a413a
Author: Ian Rogers <irogers@google.com>
perf ui hists: Remove duplicated thread in popup_action
This commit removes the thread field from struct popup_action as it is
already duplicated within the map_symbol structure. This ensures there is
only a single thread associated with a popup_action.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 6145c7878c648..89c3d3dc69a27 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2359,7 +2359,6 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
>
> struct popup_action {
> unsigned long time;
> - struct thread *thread;
[Severity: Low]
Is it possible this removal causes a compilation error? There appear to be
remaining references to the thread field in this file:
tools/perf/ui/browsers/hists.c:do_zoom_thread() {
...
struct thread *thread = act->thread;
...
}
tools/perf/ui/browsers/hists.c:do_run_script() {
...
if (act->thread) {
...
}
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
} else if (top == &browser->hists->thread_filter) {
actions->thread = thread;
do_zoom_thread(browser, actions);
...
}
[Severity: Medium]
Will this lead to unbalanced reference counting on the map object?
In the K_LEFT hotkey handler, an un-referenced map pointer is assigned into
the actions array before a continue statement:
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
actions->ms.map = map;
top = pstack__peek(browser->pstack);
if (top == &browser->hists->dso_filter) {
/*
* No need to set actions->dso here since
* it's just to remove the current filter.
*/
do_zoom_dso(browser, actions);
} else if (top == &browser->hists->thread_filter) {
actions->thread = thread;
do_zoom_thread(browser, actions);
} else if (top == &browser->hists->socket_filter) {
do_zoom_socket(browser, actions);
}
continue;
...
}
When the loop restarts at do_hotkey, free_popup_actions() drops references
for the actions array, which appears to prematurely free the map object:
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
do_hotkey: // key came straight from options ui__popup_menu()
free_popup_options(options, MAX_OPTIONS);
free_popup_actions(actions, MAX_OPTIONS);
...
}
> int (*fn)(struct hist_browser *browser, struct popup_action *act);
> struct map_symbol ms;
> int socket;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=8
^ permalink raw reply [flat|nested] 89+ 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] " 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:58 ` sashiko-bot
2026-07-10 5:36 ` [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers
` (2 subsequent siblings)
10 siblings, 1 reply; 89+ 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] 89+ messages in thread* Re: [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup
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:58 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 5:58 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Unconditional free of non-heap memory causes a crash when exiting the browser or replacing a command-line symbol filter.
- [Medium] Unconditionally clearing hists filters and un-eliding columns on browser exit permanently destroys persistent command-line configurations.
--
--- Patch [9]: [PATCH v5 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup ---
commit 1af645505e5c140a87449557e914e317c6cf9b3b
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix dso_filter reference leak and exit zoom cleanup
In hists_browser__zoom_map(), the active dso is assigned as a raw pointer
to hists->dso_filter without acquiring a reference using dso__get(). This
patch properly acquires a reference when assigning the filter, and releases
it when clearing it, while also resetting the persistent hists structure
upon exit.
Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 89c3d3dc69a27..e0ef756d95367 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -3491,6 +3492,16 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> out_free_stack:
> pstack__delete(browser->pstack);
> free_popup_actions(actions, MAX_OPTIONS);
> + thread__zput(hists->thread_filter);
> + dso__put((struct dso *)hists->dso_filter);
> + hists->dso_filter = NULL;
> + hists->socket_filter = -1;
> + perf_hpp__set_elide(HISTC_DSO, false);
> + perf_hpp__set_elide(HISTC_THREAD, false);
> + perf_hpp__set_elide(HISTC_SOCKET, false);
> + hists__filter_by_dso(hists);
> + hists__filter_by_thread(hists);
> + hists__filter_by_socket(hists);
[Severity: Medium]
Could unconditionally clearing the hists filters and un-eliding columns here
permanently destroy persistent command-line configurations?
If a user launches perf report with persistent global filters like
--socket-filter 0, hists->socket_filter is initialized. When navigating into
the TUI for a specific event and then exiting back to the main menu (tab
switching), this teardown path unconditionally clears the filters (e.g.,
hists->socket_filter = -1) and un-elides the columns.
Could this destroy the user's requested command-line configuration for the
remainder of the session?
> zfree(&hists->symbol_filter_str);
[Severity: Medium]
Will this unconditional free cause a crash when exiting the browser or
replacing a command-line symbol filter?
When a user runs perf report --symbol-filter <sym>, the argv pointer is
assigned directly without strdup():
tools/perf/builtin-report.c:cmd_report() {
...
report.symbol_filter_str = argv[0];
...
}
When exiting the browser, or when applying a new inline filter inside the TUI
via evsel__hists_browse(), this zfree() attempts to free the non-heap argv
string. Can this trigger a glibc abort?
This unconditional free was introduced by commit "perf ui hists: Fix stack
use-after-return in symbol_filter_str".
> hists__filter_by_symbol(hists);
> out:
> hist_browser__delete(browser);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=9
^ permalink raw reply [flat|nested] 89+ 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] " 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
2026-07-10 6:08 ` sashiko-bot
2026-07-10 5:54 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow sashiko-bot
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
10 siblings, 1 reply; 89+ 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] 89+ messages in thread* Re: [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread
2026-07-10 5:36 ` [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers
@ 2026-07-10 6:08 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 6:08 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Double-put of the `map` struct leading to a use-after-free when zooming out via the K_ESC/K_LEFT hotkeys.
--
--- Patch [10]: [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread ---
commit 380963af7e8bad2a7eee6ec6e6e6656a899a7aa7
Author: Ian Rogers <irogers@google.com>
perf annotate: Be robust to annotating without a thread
This commit makes perf annotate more robust by trying harder to determine
the architecture for disassembly when a thread isn't given. It introduces
map_symbol__get_arch() to replace the now removed thread__get_arch() and
safely handles map_symbol fallbacks.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index e0ef756d95367..aee3d2c4e1867 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -3341,14 +3341,16 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> actions->ms.map = map;
[Severity: Medium]
Could this assignment to actions->ms.map lead to a use-after-free?
When zooming out via the K_ESC or K_LEFT hotkeys, actions->ms.map is assigned
the current map pointer without acquiring a reference via map__get().
The loop then continues to the top, where free_popup_actions() is called on
the actions array.
Does free_popup_actions() systematically call map_symbol__exit(), causing it
to improperly drop a reference on actions[0].ms.map? This could lead to a
reference count underflow and use-after-free of the map.
> top = pstack__peek(browser->pstack);
> if (top == &browser->hists->dso_filter) {
> /*
> * No need to set actions->dso here since
> * it's just to remove the current filter.
> */
> do_zoom_dso(browser, actions);
> } else if (top == &browser->hists->thread_filter) {
> - actions->thread = thread;
> - do_zoom_thread(browser, actions);
> + memset(&hotkey_act, 0, sizeof(hotkey_act));
> + hotkey_act.ms.thread = thread__get(thread);
> + do_zoom_thread(browser, &hotkey_act);
> + map_symbol__exit(&hotkey_act.ms);
> } else if (top == &browser->hists->socket_filter) {
> do_zoom_socket(browser, actions);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=10
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow
2026-07-10 5:36 ` [PATCH v5 01/10] " Ian Rogers
` (8 preceding siblings ...)
2026-07-10 5:36 ` [PATCH v5 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers
@ 2026-07-10 5:54 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
10 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-10 5:54 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] A pointer to a local stack array is assigned to a longer-lived structure field (`hists->symbol_filter_str`), leading to a Use-After-Free.
--
--- Patch [1]: [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow ---
commit b1c79545734c108ba31495cc1454bc12dd7ce4ad
Author: Ian Rogers <irogers@google.com>
perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow
In evsel__hists_browse(), the options and actions arrays are statically
allocated on the stack with a size of MAX_OPTIONS. Depending on the context,
the sum of added options can theoretically exceed 16, potentially leading to
a stack buffer overflow. This patch increases MAX_OPTIONS to 32 to safely
accommodate the maximum possible number of options.
Link: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index cfa6386e6e1da..671961946d56c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3003,7 +3003,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> struct hists *hists = evsel__hists(evsel);
> struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
> struct branch_info *bi = NULL;
> -#define MAX_OPTIONS 16
> +#define MAX_OPTIONS 32
> char *options[MAX_OPTIONS];
> struct popup_action actions[MAX_OPTIONS];
> int nr_options = 0;
[Severity: High]
This isn't a bug introduced by this patch, but since we are looking at
evsel__hists_browse(), can this cause a use-after-free later?
Just below this context, there is a local stack array declared:
char buf[128];
Later in the function, when handling the '/' key for symbol filtering,
a pointer to this local stack array is assigned to a persistent field:
case '/':
if (ui_browser__input_window("Symbol to show",
...
buf, "ENTER: OK, ESC: Cancel",
delay_secs * 2) == K_ENTER) {
hists->symbol_filter_str = *buf ? buf : NULL;
...
When the function returns and the user exits the browser, buf is destroyed,
but hists->symbol_filter_str will still point to this invalid stack memory.
In a multi-threaded context like perf top, if background processing threads or
subsequent timer-driven UI refreshes (via hists__filter_by_symbol() ->
hists__filter_entry_by_symbol() -> strstr()) read from it:
strstr(he->ms.sym->name, hists->symbol_filter_str)
Could this lead to memory corruption or crashes?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260710053628.1861645-1-irogers@google.com?part=1
^ permalink raw reply [flat|nested] 89+ messages in thread* [PATCH v6 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow
2026-07-10 5:36 ` [PATCH v5 01/10] " Ian Rogers
` (9 preceding siblings ...)
2026-07-10 5:54 ` [PATCH v5 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow sashiko-bot
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:23 ` [PATCH v6 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers
` (10 more replies)
10 siblings, 11 replies; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* [PATCH v6 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:40 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 03/10] perf ui hists: Include limits.h for PATH_MAX definition Ian Rogers
` (9 subsequent siblings)
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure
2026-07-16 7:23 ` [PATCH v6 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers
@ 2026-07-16 7:40 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:40 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] `hists->symbol_filter_str` points to a local stack array, leading to a dangling pointer Use-After-Free when the function returns.
--
--- Patch [2]: [PATCH v6 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure ---
commit ee0809111abadd84f6d0242516fc731b6121c8a4
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix uninitialized stack memory free on pstack allocation failure
This commit fixes a potential heap corruption issue in the hists browser. It
ensures that the options and actions arrays are initialized prior to allocating
the pstack, which prevents uninitialized stack pointers from being freed if the
allocation fails.
Closes: https://lore.kernel.org/linux-perf-users/20260709035230.6DBEE1F000E9@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 671961946d56c..45958bbe45092 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3064,15 +3064,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> browser->min_pcnt = min_pcnt;
> hist_browser__update_nr_entries(browser);
>
> + memset(options, 0, sizeof(options));
> + memset(actions, 0, sizeof(actions));
> +
> browser->pstack = pstack__new(3);
> if (browser->pstack == NULL)
> goto out;
>
> ui_helpline__push(helpline);
>
> - memset(options, 0, sizeof(options));
> - memset(actions, 0, sizeof(actions));
> -
> if (symbol_conf.col_width_list_str)
> perf_hpp__set_user_width(symbol_conf.col_width_list_str);
>
[Severity: High]
This isn't a bug introduced by this patch, but while looking at
evsel__hists_browse(), is it safe to assign the local stack array buf to
hists->symbol_filter_str?
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
char buf[128];
...
case '/':
if (ui_browser__input_window(..., buf, ...)) {
hists->symbol_filter_str = *buf ? buf : NULL;
hists__filter_by_symbol(hists);
hist_browser__reset(browser);
}
...
}
Since buf is allocated on the stack, the pointer becomes invalid once
evsel__hists_browse() returns.
If the user exits the browser and the background timer later fires while in the
parent menu, the timer executes:
perf_top__sort_new_samples()
perf_top__resort_hists()
hists__collapse_resort()
hists__apply_filters()
Could this result in hists__apply_filters() reading from the dangling stack
pointer and causing a crash?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=2
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v6 03/10] perf ui hists: Include limits.h for PATH_MAX definition
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
2026-07-16 7:23 ` [PATCH v6 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:23 ` [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers
` (8 subsequent siblings)
10 siblings, 0 replies; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
2026-07-16 7:23 ` [PATCH v6 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure Ian Rogers
2026-07-16 7:23 ` [PATCH v6 03/10] perf ui hists: Include limits.h for PATH_MAX definition Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:48 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() Ian Rogers
` (7 subsequent siblings)
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str
2026-07-16 7:23 ` [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers
@ 2026-07-16 7:48 ` sashiko-bot
2026-07-18 5:38 ` Namhyung Kim
0 siblings, 1 reply; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:48 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional free of a non-heap `argv` pointer leading to a crash, and unintended destruction of the CLI symbol filter state.
--
--- Patch [4]: [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str ---
commit 7f822e6fd8f11b66dda82bea74a7a54f3a875831
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix stack use-after-return in symbol_filter_str
In evsel__hists_browse(), the local stack array 'buf' is assigned directly
to the persistent 'hists->symbol_filter_str' pointer. This commit fixes the
dangling pointer by duplicating the string and cleanly freeing it on exit.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 793959574a9ad..4282ef1b81f75 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3213,7 +3213,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> "To remove the filter later, press / + ENTER.",
> buf, "ENTER: OK, ESC: Cancel",
> delay_secs * 2) == K_ENTER) {
> - hists->symbol_filter_str = *buf ? buf : NULL;
> + char *new_filter = *buf ? strdup(buf) : NULL;
> +
> + zfree(&hists->symbol_filter_str);
[Severity: High]
If the user invoked perf report with a command line option like
--symbol-filter=my_func, hists->symbol_filter_str will have been assigned
directly to the argv pointer in tools/perf/builtin-report.c:
report__collapse_hists() {
...
hists->symbol_filter_str = rep->symbol_filter_str;
Will calling zfree() here cause a glibc invalid pointer crash when the
user interactively applies a new filter?
> + hists->symbol_filter_str = new_filter;
> hists__filter_by_symbol(hists);
> hist_browser__reset(browser);
> }
[ ... ]
> @@ -3447,6 +3450,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> }
> out_free_stack:
> pstack__delete(browser->pstack);
> + zfree(&hists->symbol_filter_str);
> + hists__filter_by_symbol(hists);
[Severity: High]
Similarly, does this unconditional free on exit crash if the filter was
initialized from the CLI?
Additionally, does unconditionally clearing this pointer permanently destroy
the --symbol-filter state for subsequent views of the same event?
> out:
> hist_browser__delete(browser);
> free_popup_options(options, MAX_OPTIONS);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=4
^ permalink raw reply [flat|nested] 89+ messages in thread* Re: [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str
2026-07-16 7:48 ` sashiko-bot
@ 2026-07-18 5:38 ` Namhyung Kim
0 siblings, 0 replies; 89+ messages in thread
From: Namhyung Kim @ 2026-07-18 5:38 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Ian Rogers, linux-perf-users
On Thu, Jul 16, 2026 at 07:48:47AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unconditional free of a non-heap `argv` pointer leading to a crash, and unintended destruction of the CLI symbol filter state.
> --
>
> --- Patch [4]: [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str ---
> commit 7f822e6fd8f11b66dda82bea74a7a54f3a875831
> Author: Ian Rogers <irogers@google.com>
>
> perf ui hists: Fix stack use-after-return in symbol_filter_str
>
> In evsel__hists_browse(), the local stack array 'buf' is assigned directly
> to the persistent 'hists->symbol_filter_str' pointer. This commit fixes the
> dangling pointer by duplicating the string and cleanly freeing it on exit.
>
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index 793959574a9ad..4282ef1b81f75 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -3213,7 +3213,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> > "To remove the filter later, press / + ENTER.",
> > buf, "ENTER: OK, ESC: Cancel",
> > delay_secs * 2) == K_ENTER) {
> > - hists->symbol_filter_str = *buf ? buf : NULL;
> > + char *new_filter = *buf ? strdup(buf) : NULL;
> > +
> > + zfree(&hists->symbol_filter_str);
>
> [Severity: High]
> If the user invoked perf report with a command line option like
> --symbol-filter=my_func, hists->symbol_filter_str will have been assigned
> directly to the argv pointer in tools/perf/builtin-report.c:
>
> report__collapse_hists() {
> ...
> hists->symbol_filter_str = rep->symbol_filter_str;
>
> Will calling zfree() here cause a glibc invalid pointer crash when the
> user interactively applies a new filter?
Right, please update there to copy the command line string. It seems
there's a test code to set it too.
Thanks,
Namhyung
>
> > + hists->symbol_filter_str = new_filter;
> > hists__filter_by_symbol(hists);
> > hist_browser__reset(browser);
> > }
>
> [ ... ]
>
> > @@ -3447,6 +3450,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> > }
> > out_free_stack:
> > pstack__delete(browser->pstack);
> > + zfree(&hists->symbol_filter_str);
> > + hists__filter_by_symbol(hists);
>
> [Severity: High]
> Similarly, does this unconditional free on exit crash if the filter was
> initialized from the CLI?
>
> Additionally, does unconditionally clearing this pointer permanently destroy
> the --symbol-filter state for subsequent views of the same event?
>
> > out:
> > hist_browser__delete(browser);
> > free_popup_options(options, MAX_OPTIONS);
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=4
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v6 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find()
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (2 preceding siblings ...)
2026-07-16 7:23 ` [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:40 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers
` (6 subsequent siblings)
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find()
2026-07-16 7:23 ` [PATCH v6 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() Ian Rogers
@ 2026-07-16 7:40 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:40 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [Low] Missing explicit inclusions of POSIX standard headers `<stdio.h>`, `<limits.h>`, and `<string.h>`.
--
--- Patch [5]: [PATCH v6 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() ---
commit ca1fd445ff68f5d4f2326b24e16c260e3dc0cb4f
Author: Ian Rogers <irogers@google.com>
perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find()
This commit fixes a NULL pointer dereference in the error path of
arch__find() by printing the e_machine ID instead of the uninitialized
name. It also prevents a use-after-free by correctly updating the global
archs pointer immediately after successfully reallocating memory.
Closes: https://lore.kernel.org/linux-perf-users/20260709035721.9EE901F000E9@smtp.kernel.org/
> diff --git a/tools/perf/util/disasm.c b/tools/perf/util/disasm.c
> index 0a1a7e9cf3efe..6cfdbabbb8c7d 100644
> --- a/tools/perf/util/disasm.c
> +++ b/tools/perf/util/disasm.c
[Severity: Low]
This isn't a bug introduced by this patch, but there is a pre-existing
issue in the include block at the top of tools/perf/util/disasm.c.
Because musl libc strictly separates declarations unlike glibc, missing
explicit inclusions of POSIX standard headers such as <stdio.h>, <limits.h>,
and <string.h> can cause compilation failures when building the perf tool
in a musl environment.
Could we add these explicit headers at the top of the file to ensure
consistent musl compatibility during the build?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=5
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt()
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (3 preceding siblings ...)
2026-07-16 7:23 ` [PATCH v6 05/10] perf disasm: Fix potential NULL pointer dereference and use-after-free in arch__find() Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:39 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 07/10] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers
` (5 subsequent siblings)
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt()
2026-07-16 7:23 ` [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers
@ 2026-07-16 7:39 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:39 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [Medium] Memory leak of dynamically allocated strings for the popup menu in `evsel__hists_browse`.
- [Medium] NULL pointer dereference when computing script option time range for an empty trace.
--
--- Patch [6]: [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() ---
commit 7d70d8b9859cd9c3627530a34fb514614a33b89f
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix NULL pointer array gap in add_script_opt()
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, this leaves a NULL pointer gap in the options array. When
ui__popup_menu() is later displayed, it dereferences this gap and crashes.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 4282ef1b81f75..46fb484a4dc64 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2811,7 +2811,7 @@ add_script_opt(struct hist_browser *browser,
> struct popup_action *act, char **optstr,
> struct thread *thread, struct symbol *sym)
> {
> - int n, j;
> + int n, j, ret;
> struct hist_entry *he;
>
> n = add_script_opt_2(act, optstr, thread, sym, "");
[Severity: Medium]
This is a pre-existing issue, but does this dynamically allocated string leak?
When repeatedly opening and closing the popup menu (pressing 'm' then 'ESC'),
evsel__hists_browse() resets nr_options to 0 inside the event loop.
This overwrites the options array without calling free_popup_options() to clean
up the strings allocated by asprintf() in add_script_opt_2().
>
> he = hist_browser__selected_entry(browser);
> if (sort_order && strstr(sort_order, "time")) {
> char tstr[128];
> + struct popup_action *time_act = act;
> + char **time_optstr = optstr;
>
> - optstr++;
> - act++;
> + if (n > 0) {
> + time_optstr++;
> + time_act++;
> + }
> j = sprintf(tstr, " in ");
> j += timestamp__scnprintf_usec(he->time, tstr + j,
> sizeof tstr - j);
[Severity: Medium]
This is a pre-existing issue, but can he be NULL here leading to a crash?
If the trace is empty (e.g., all entries filtered out) and the user presses 'm',
evsel__hists_browse() calls add_script_opt() unconditionally, even if
hist_browser__selected_entry() returns NULL.
> 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;
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=6
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v6 07/10] perf ui hists: In report UI ensure thread is set with reference counting
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (4 preceding siblings ...)
2026-07-16 7:23 ` [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:35 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
` (4 subsequent siblings)
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 07/10] perf ui hists: In report UI ensure thread is set with reference counting
2026-07-16 7:23 ` [PATCH v6 07/10] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers
@ 2026-07-16 7:35 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:35 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Thread zooming and thread-specific scripting silently fail because consumer functions were not updated to read the new reference-counted `act->ms.thread` field.
- [Medium] The 'Zoom out' hotkey handler (`K_ESC` / `K_LEFT`) causes an unbalanced reference count decrement on a `struct map`, leading to a use-after-free.
--
--- Patch [7]: [PATCH v6 07/10] perf ui hists: In report UI ensure thread is set with reference counting ---
commit 9d863ec6315c574f4f236ffe39082f794838058e
Author: Ian Rogers <irogers@google.com>
perf ui hists: In report UI ensure thread is set with reference counting
Populates the map_symbol thread in the UI code to fix the e_machine
lookup for cross-platform disassembly when using the annotate action.
At the same time, refactor the UI options and actions generation to
comply with the perf subsystem's strict reference counting abstraction
requirements for 'struct map_symbol' and 'struct thread'.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 46fb484a4dc64..6145c7878c648 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -2628,7 +2636,7 @@ add_thread_opt(struct hist_browser *browser, struct popup_action *act,
> if (ret < 0)
> return 0;
>
> - act->thread = thread;
> + act->ms.thread = thread__get(thread);
> act->fn = do_zoom_thread;
> return 1;
> }
[Severity: Medium]
Does this change break thread zooming and thread-specific scripting?
The struct popup_action still contains the thread field, and consumer functions
like do_zoom_thread() and do_run_script() still check act->thread to append
arguments. Because act variables are zero-initialized, act->thread remains
NULL and the functions might silently abort or skip thread filtering.
For example, do_run_script() checks this field:
if (act->thread)
len += strlen(thread__comm_str(act->thread));
Should these consumer functions be updated to read act->ms.thread instead?
[ ... ]
> @@ -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);
[Severity: Medium]
Can this free_popup_actions() call cause a use-after-free by decrementing the
reference count of an unowned map?
If the user presses K_ESC or K_LEFT to zoom out, evsel__hists_browse() assigns
a borrowed map reference to the first actions array element:
case K_ESC:
case K_LEFT: {
...
actions->ms.map = map;
top = pstack__peek(browser->pstack);
...
The loop can then continue back to the top, where free_popup_actions() is
unconditionally called. This triggers map_symbol__exit() on actions[0],
causing map__zput() to decrement the reference count of the unowned map.
> choice = nr_options = 0;
> key = hist_browser__run(browser, helpline, warn_lost_event, key);
>
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=7
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (5 preceding siblings ...)
2026-07-16 7:23 ` [PATCH v6 07/10] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:37 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers
` (3 subsequent siblings)
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action
2026-07-16 7:23 ` [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
@ 2026-07-16 7:37 ` sashiko-bot
2026-07-18 5:38 ` Namhyung Kim
0 siblings, 1 reply; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:37 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Assigning a raw pointer to `actions->ms.map` without taking a reference, followed by `free_popup_actions()` dropping the reference, causes a refcount underflow.
- [Low] The commit removes the `thread` member from `struct popup_action` but leaves multiple read and write accesses to it intact, breaking the build.
--
--- Patch [8]: [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action ---
commit 1a6678f7f83293dd5e22da61354ad80c36378c0a
Author: Ian Rogers <irogers@google.com>
perf ui hists: Remove duplicated thread in popup_action
This commit removes the thread member from struct popup_action since it is
duplicated in the map_symbol structure. It aims to ensure there is only ever
one thread associated with a popup_action.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 6145c7878c648..89c3d3dc69a27 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2359,7 +2359,6 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
>
> struct popup_action {
> unsigned long time;
> - struct thread *thread;
[Severity: Low]
Does this removal cause a build regression?
There are still accesses to act->thread and actions->thread in several places
that will no longer compile:
tools/perf/ui/browsers/hists.c:do_zoom_thread() {
struct thread *thread = act->thread;
...
}
tools/perf/ui/browsers/hists.c:do_run_script() {
...
if (act->thread)
len += strlen(thread__comm_str(act->thread));
...
}
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
} else if (top == &browser->hists->thread_filter) {
actions->thread = thread;
do_zoom_thread(browser, actions);
}
...
}
[Severity: Medium]
Can this code cause a refcount underflow regression?
In evsel__hists_browse(), when handling K_ESC or K_LEFT, actions->ms.map is
assigned directly without taking a reference:
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
case K_ESC:
case K_LEFT: {
...
actions->ms.map = map;
...
continue;
}
...
}
The loop then hits continue, and at the do_hotkey label, free_popup_actions()
drops the reference, which can cause an underflow when map_symbol__exit()
calls map__zput():
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
do_hotkey:
free_popup_options(options, MAX_OPTIONS);
free_popup_actions(actions, MAX_OPTIONS);
...
}
> int (*fn)(struct hist_browser *browser, struct popup_action *act);
> struct map_symbol ms;
> int socket;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=8
^ permalink raw reply [flat|nested] 89+ messages in thread* Re: [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action
2026-07-16 7:37 ` sashiko-bot
@ 2026-07-18 5:38 ` Namhyung Kim
0 siblings, 0 replies; 89+ messages in thread
From: Namhyung Kim @ 2026-07-18 5:38 UTC (permalink / raw)
To: sashiko-reviews; +Cc: Ian Rogers, linux-perf-users
On Thu, Jul 16, 2026 at 07:37:50AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [Medium] Assigning a raw pointer to `actions->ms.map` without taking a reference, followed by `free_popup_actions()` dropping the reference, causes a refcount underflow.
> - [Low] The commit removes the `thread` member from `struct popup_action` but leaves multiple read and write accesses to it intact, breaking the build.
> --
>
> --- Patch [8]: [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action ---
> commit 1a6678f7f83293dd5e22da61354ad80c36378c0a
> Author: Ian Rogers <irogers@google.com>
>
> perf ui hists: Remove duplicated thread in popup_action
>
> This commit removes the thread member from struct popup_action since it is
> duplicated in the map_symbol structure. It aims to ensure there is only ever
> one thread associated with a popup_action.
>
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index 6145c7878c648..89c3d3dc69a27 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -2359,7 +2359,6 @@ static int hists_browser__scnprintf_title(struct hist_browser *browser, char *bf
> >
> > struct popup_action {
> > unsigned long time;
> > - struct thread *thread;
>
> [Severity: Low]
> Does this removal cause a build regression?
Right, this should be the last patch in the series.
Thanks,
Namhyung
>
> There are still accesses to act->thread and actions->thread in several places
> that will no longer compile:
>
> tools/perf/ui/browsers/hists.c:do_zoom_thread() {
> struct thread *thread = act->thread;
> ...
> }
>
> tools/perf/ui/browsers/hists.c:do_run_script() {
> ...
> if (act->thread)
> len += strlen(thread__comm_str(act->thread));
> ...
> }
>
> tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
> ...
> } else if (top == &browser->hists->thread_filter) {
> actions->thread = thread;
> do_zoom_thread(browser, actions);
> }
> ...
> }
>
> [Severity: Medium]
> Can this code cause a refcount underflow regression?
>
> In evsel__hists_browse(), when handling K_ESC or K_LEFT, actions->ms.map is
> assigned directly without taking a reference:
>
> tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
> ...
> case K_ESC:
> case K_LEFT: {
> ...
> actions->ms.map = map;
> ...
> continue;
> }
> ...
> }
>
> The loop then hits continue, and at the do_hotkey label, free_popup_actions()
> drops the reference, which can cause an underflow when map_symbol__exit()
> calls map__zput():
>
> tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
> ...
> do_hotkey:
> free_popup_options(options, MAX_OPTIONS);
> free_popup_actions(actions, MAX_OPTIONS);
> ...
> }
>
> > int (*fn)(struct hist_browser *browser, struct popup_action *act);
> > struct map_symbol ms;
> > int socket;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=8
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v6 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (6 preceding siblings ...)
2026-07-16 7:23 ` [PATCH v6 08/10] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:49 ` sashiko-bot
2026-07-16 7:23 ` [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers
` (2 subsequent siblings)
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup
2026-07-16 7:23 ` [PATCH v6 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers
@ 2026-07-16 7:49 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:49 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Calling `zfree(&hists->symbol_filter_str)` on a pointer that originated from an unallocated command-line argument causes a fatal crash.
- [Medium] The newly added exit cleanup blindly clears global command-line configurations like `socket_filter`, permanently destroying their effects across browser tabs.
--
--- Patch [9]: [PATCH v6 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup ---
commit 8368ef0112285b0a616e2a03e40dea893c6d1d5b
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix dso_filter reference leak and exit zoom cleanup
This commit fixes a potential use-after-free by acquiring and releasing a
reference to dso_filter in hists_browser__zoom_map(). It also fully resets
the persistent hists structure upon exiting the browser stack to prevent
state corruption across tab switches.
Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 89c3d3dc69a27..e0ef756d95367 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -3491,6 +3492,16 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> out_free_stack:
> pstack__delete(browser->pstack);
> free_popup_actions(actions, MAX_OPTIONS);
> + thread__zput(hists->thread_filter);
> + dso__put((struct dso *)hists->dso_filter);
> + hists->dso_filter = NULL;
> + hists->socket_filter = -1;
[Severity: Medium]
Will this unconditionally clear the global command-line configuration?
If a user invokes perf report with --socket-filter <N>, the configuration
is stored in hists->socket_filter. When the user exits the TUI view (e.g.,
by pressing ESC), this cleanup resets the filter to -1.
Could this result in subsequent views of the same event incorrectly
displaying unfiltered data across browser tabs?
> + 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);
[Severity: High]
Does this zfree() call attempt to free a non-heap pointer?
If a user invokes perf report --symbol-filter <sym>, the command-line
argument is assigned without dynamic allocation:
tools/perf/builtin-report.c:cmd_report() {
...
report.symbol_filter_str = argv[0];
...
}
This pointer is then passed to hists->symbol_filter_str. When the TUI exits
evsel__hists_browse(), calling zfree() on this non-heap pointer will
trigger a fatal glibc abort. Can we avoid calling zfree() on pointers
originating from unallocated command-line arguments?
> hists__filter_by_symbol(hists);
> out:
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=9
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (7 preceding siblings ...)
2026-07-16 7:23 ` [PATCH v6 09/10] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers
@ 2026-07-16 7:23 ` Ian Rogers
2026-07-16 7:51 ` sashiko-bot
2026-07-20 4:45 ` [PATCH v6 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Namhyung Kim
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
10 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-16 7:23 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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 | 48 ++++++++++++++++++++++---------
tools/perf/util/annotate.h | 3 +-
tools/perf/util/capstone.c | 42 +++++++++++++++++++++------
5 files changed, 81 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..af4e8afcf82b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -982,24 +982,40 @@ 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;
+ const char *cpuid = NULL;
+
+ if (ms->thread) {
+ machine = maps__machine(thread__maps(ms->thread));
+ e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+ if (machine && machine->env)
+ cpuid = machine->env->cpuid;
+ } else if (dso) {
+ struct maps *kmaps = map ? map__kmaps(map) : NULL;
+ struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
+
+ e_machine = dso__e_machine(dso, kmap_machine, &e_flags);
+ if (kmap_machine && kmap_machine->env)
+ cpuid = kmap_machine->env->cpuid;
}
- 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, cpuid);
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 +1034,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 +1267,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 +1287,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 +1391,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 9bba78ee0c5a..758ce837b373 100644
--- a/tools/perf/util/capstone.c
+++ b/tools/perf/util/capstone.c
@@ -385,7 +385,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)
@@ -416,9 +416,22 @@ 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) {
+ struct maps *kmaps = map ? map__kmaps(map) : NULL;
+ struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
+
+ e_machine = dso__e_machine_endian(dso, kmap_machine, /*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;
@@ -511,7 +524,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)
@@ -531,9 +544,22 @@ 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) {
+ struct maps *kmaps = map ? map__kmaps(map) : NULL;
+ struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
+
+ e_machine = dso__e_machine_endian(dso, kmap_machine, /*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.141.g00534a21ce-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread
2026-07-16 7:23 ` [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers
@ 2026-07-16 7:51 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-16 7:51 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Unconditional call to `map__kmaps()` on user-space maps triggers internal error spam.
- [High] Unbalanced `map__put()` on the selected `map` when zooming out via ESC/LEFT leads to Use-After-Free.
- [Medium] `map_symbol__get_arch` returns negative POSIX error code, breaking its caller's error formatting.
--
--- Patch [10]: [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread ---
commit 6b7b7896027a6d84296eafef08984d82adf8db72
Author: Ian Rogers <irogers@google.com>
perf annotate: Be robust to annotating without a thread
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.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index e0ef756d95367..aee3d2c4e1867 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
[ ... ]
> @@ -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);
[Severity: High]
Does the migration to use map_symbol in popup_action elements introduce a
use-after-free when zooming out?
In evsel__hists_browse(), when handling K_ESC or K_LEFT to zoom out, the
code assigns the map pointer directly:
evsel__hists_browse() {
...
case K_ESC:
case K_LEFT: {
...
actions->ms.map = map;
top = pstack__peek(browser->pstack);
...
continue;
}
This assigns the raw map pointer without claiming a reference via map__get().
When the loop continues, the top of the loop unconditionally executes
free_popup_actions(actions, MAX_OPTIONS) to clean up the array. Since the
struct now embeds a map_symbol, this calls map_symbol__exit() which will
execute map__zput() on the un-refcounted map.
Could this unbalanced put prematurely free the map while it is still
referenced elsewhere?
> } 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 02505222d8c2d..deb92c27f7abf 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -982,24 +982,40 @@ 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;
> + const char *cpuid = NULL;
> +
> + if (ms->thread) {
> + machine = maps__machine(thread__maps(ms->thread));
> + e_machine = thread__e_machine(ms->thread, machine, &e_flags);
> + if (machine && machine->env)
> + cpuid = machine->env->cpuid;
> + } else if (dso) {
> + struct maps *kmaps = map ? map__kmaps(map) : NULL;
[Severity: High]
Does this call to map__kmaps() verify if the map is actually a kernel map
first?
If map__kmaps() is invoked on a user-space map, it forcefully prints an
internal error and returns NULL. Since perf annotate might process many
user-space symbols, could this result in continuous console spam that
obscures real output and breaks the TUI layout?
Should this check dso__kernel(dso) before attempting to extract the kernel
maps structure?
> + struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> + e_machine = dso__e_machine(dso, kmap_machine, &e_flags);
> + if (kmap_machine && kmap_machine->env)
> + cpuid = kmap_machine->env->cpuid;
> }
>
> - 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, cpuid);
> if (arch == NULL) {
> pr_err("%s: unsupported arch %d\n", __func__, e_machine);
> - return errno;
> + return errno ? errno : -ENOTSUP;
[Severity: Medium]
Is returning a negative POSIX error code (-ENOTSUP) expected by the caller?
The caller symbol__strerror_disassemble() seems to expect POSIX system error
codes to be passed as positive integers to correctly translate them with
str_error_r(). It uses negative integers exclusively for custom
SYMBOL_ANNOTATE_ERRNO__* constants.
If we return -95 here, does the switch statement in the error formatter
default to printing an invalid error code message instead of the real
unsupported architecture error?
> }
> if (parch)
> *parch = arch;
[ ... ]
> diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> index 5ad537fea4360..b4903f351bac0 100644
> --- a/tools/perf/util/capstone.c
> +++ b/tools/perf/util/capstone.c
[ ... ]
> @@ -411,9 +411,22 @@ 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) {
> + struct maps *kmaps = map ? map__kmaps(map) : NULL;
[Severity: High]
Similarly to map_symbol__get_arch(), does this unconditionally invoke
map__kmaps() on what could be a user-space map, triggering the same console
spam issue?
> + struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> + e_machine = dso__e_machine_endian(dso, kmap_machine, /*e_flags=*/NULL,
> + &is_big_endian);
> + }
[ ... ]
> @@ -526,9 +539,22 @@ 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) {
> + struct maps *kmaps = map ? map__kmaps(map) : NULL;
[Severity: High]
Is the same dso__kernel(dso) check missing here as well?
> + struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> + e_machine = dso__e_machine_endian(dso, kmap_machine, /*e_flags=*/NULL,
> + &is_big_endian);
> + }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=10
^ permalink raw reply [flat|nested] 89+ messages in thread
* Re: [PATCH v6 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (8 preceding siblings ...)
2026-07-16 7:23 ` [PATCH v6 10/10] perf annotate: Be robust to annotating without a thread Ian Rogers
@ 2026-07-20 4:45 ` Namhyung Kim
2026-07-22 16:37 ` Ian Rogers
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
10 siblings, 1 reply; 89+ messages in thread
From: Namhyung Kim @ 2026-07-20 4:45 UTC (permalink / raw)
To: Ian Rogers
Cc: acme, adrian.hunter, dapeng1.mi, james.clark, jistone, jolsa,
linux-kernel, linux-perf-users, mingo, peterz, tianyou.li
On Thu, Jul 16, 2026 at 12:23:42AM -0700, Ian Rogers wrote:
> 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.
Applied patch 1 to 6 (except 4) to perf-tools-next, thanks!
Best regards,
Namhyung
^ permalink raw reply [flat|nested] 89+ messages in thread* Re: [PATCH v6 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow
2026-07-20 4:45 ` [PATCH v6 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Namhyung Kim
@ 2026-07-22 16:37 ` Ian Rogers
0 siblings, 0 replies; 89+ messages in thread
From: Ian Rogers @ 2026-07-22 16:37 UTC (permalink / raw)
To: Namhyung Kim
Cc: acme, adrian.hunter, dapeng1.mi, james.clark, jistone, jolsa,
linux-kernel, linux-perf-users, mingo, peterz, tianyou.li
On Sun, Jul 19, 2026 at 9:45 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Jul 16, 2026 at 12:23:42AM -0700, Ian Rogers wrote:
> > 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.
>
> Applied patch 1 to 6 (except 4) to perf-tools-next, thanks!
Thanks Namhyung. That's the main part of the fix. I'll try to follow
up on the rest (patch 4) asap.
Ian
> Best regards,
> Namhyung
>
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation
2026-07-16 7:23 ` [PATCH v6 " Ian Rogers
` (9 preceding siblings ...)
2026-07-20 4:45 ` [PATCH v6 01/10] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow Namhyung Kim
@ 2026-07-23 4:59 ` Ian Rogers
2026-07-23 4:59 ` [PATCH v7 1/6] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers
` (5 more replies)
10 siblings, 6 replies; 89+ messages in thread
From: Ian Rogers @ 2026-07-23 4:59 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, jolsa,
linux-kernel, linux-perf-users, mingo, peterz, tianyou.li
This series contains the remaining 6 unmerged patches from our UI, hists,
and annotate fix series. Upstream maintainer Namhyung Kim applied patches
1-6 (except patch 4) from the v6 series into perf-tools-next.
This v7 series includes:
- Fix stack use-after-return for symbol_filter_str by duplicating with
strdup() and freeing during evsel destruction in hists_evsel__exit().
- Guard against NULL hist_entry in add_script_opt() when trace is empty.
- Populate map_symbol thread in report UI with proper reference counting
(map_symbol__copy, thread__get) and memset zeroing in
free_popup_actions().
- Fix dso_filter reference leak and exit zoom cleanup.
- Robust cross-platform disassembly fallback when annotating without a
thread.
- Remove duplicated thread member from struct popup_action (placed at the
end of the series per reviewer feedback).
v7 Changes:
- Rebased onto latest perf-tools-next (after patches 1-3, 5-6 were
merged).
- Move symbol_filter_str teardown to hists_evsel__exit() to allow filter
persistence across TUI navigation while preventing memory leaks.
- Add memset zeroing to free_popup_actions() to prevent cross-iteration
stale action leakage.
- Placed "Remove duplicated thread in popup_action" as the final patch
in the series as requested.
Ian Rogers (6):
perf ui hists: Fix stack use-after-return in symbol_filter_str
perf ui hists: Guard against NULL hist_entry in add_script_opt()
perf ui hists: In report UI ensure thread is set with reference
counting
perf ui hists: Fix dso_filter reference leak and exit zoom cleanup
perf annotate: Be robust to annotating without a thread
perf ui hists: Remove duplicated thread in popup_action
tools/perf/builtin-report.c | 7 +-
tools/perf/ui/browsers/annotate.c | 2 +-
tools/perf/ui/browsers/hists.c | 153 ++++++++++++++++++++----------
tools/perf/util/annotate.c | 51 +++++++---
tools/perf/util/annotate.h | 3 +-
tools/perf/util/capstone.c | 42 ++++++--
tools/perf/util/hist.c | 1 +
7 files changed, 182 insertions(+), 77 deletions(-)
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply [flat|nested] 89+ messages in thread* [PATCH v7 1/6] perf ui hists: Fix stack use-after-return in symbol_filter_str
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
@ 2026-07-23 4:59 ` Ian Rogers
2026-07-23 5:17 ` sashiko-bot
2026-07-23 4:59 ` [PATCH v7 2/6] perf ui hists: Guard against NULL hist_entry in add_script_opt() Ian Rogers
` (4 subsequent siblings)
5 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-23 4:59 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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/builtin-report.c | 7 +++++--
tools/perf/ui/browsers/hists.c | 5 ++++-
tools/perf/util/hist.c | 1 +
3 files changed, 10 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
index 10db1e5f1e6c..60d1f166629e 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -726,8 +726,11 @@ static int report__collapse_hists(struct report *rep)
evlist__for_each_entry(rep->session->evlist, pos) {
struct hists *hists = evsel__hists(pos);
- if (pos->core.idx == 0)
- hists->symbol_filter_str = rep->symbol_filter_str;
+ if (pos->core.idx == 0) {
+ hists->symbol_filter_str =
+ rep->symbol_filter_str ?
+ strdup(rep->symbol_filter_str) : NULL;
+ }
hists->socket_filter = rep->socket_filter;
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index b6002724bc3a..be8a6b169722 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -3220,7 +3220,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);
}
diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index c93915625ee7..443694926f1a 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -3056,6 +3056,7 @@ static void hists_evsel__exit(struct evsel *evsel)
struct perf_hpp_list_node *node, *tmp;
hists__delete_all_entries(hists);
+ zfree(&hists->symbol_filter_str);
zfree(&hists->mem_stat_types);
zfree(&hists->mem_stat_total);
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v7 1/6] perf ui hists: Fix stack use-after-return in symbol_filter_str
2026-07-23 4:59 ` [PATCH v7 1/6] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers
@ 2026-07-23 5:17 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-23 5:17 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing check for `strdup()` allocation failure causes silent misbehavior.
--
--- Patch [1]: [PATCH v7 1/6] perf ui hists: Fix stack use-after-return in symbol_filter_str ---
commit 4572e956370768dd763a1eddb42a82592b917d6a
Author: Ian Rogers <irogers@google.com>
perf ui hists: Fix stack use-after-return in symbol_filter_str
This commit fixes a stack use-after-return bug by duplicating the filter
string with strdup() instead of assigning a local stack array. It also
ensures the memory is correctly cleaned up using zfree().
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 10db1e5f1e6c4..60d1f166629e0 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -726,8 +726,11 @@ static int report__collapse_hists(struct report *rep)
> evlist__for_each_entry(rep->session->evlist, pos) {
> struct hists *hists = evsel__hists(pos);
>
> - if (pos->core.idx == 0)
> - hists->symbol_filter_str = rep->symbol_filter_str;
> + if (pos->core.idx == 0) {
> + hists->symbol_filter_str =
> + rep->symbol_filter_str ?
> + strdup(rep->symbol_filter_str) : NULL;
[Severity: Medium]
If strdup() fails due to out-of-memory, it will return NULL. Does this silently
ignore the user's requested symbol filter without propagating the error or
warning the user?
> + }
>
> hists->socket_filter = rep->socket_filter;
>
[ ... ]
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index b6002724bc3a6..be8a6b1697229 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3220,7 +3220,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;
> +
[Severity: Medium]
If strdup() fails, new_filter will be NULL. Could this cause the user's
existing filter to be unconditionally deleted and replaced with NULL without
warning?
> + zfree(&hists->symbol_filter_str);
> + hists->symbol_filter_str = new_filter;
> hists__filter_by_symbol(hists);
> hist_browser__reset(browser);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723045949.988740-1-irogers@google.com?part=1
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v7 2/6] perf ui hists: Guard against NULL hist_entry in add_script_opt()
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
2026-07-23 4:59 ` [PATCH v7 1/6] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers
@ 2026-07-23 4:59 ` Ian Rogers
2026-07-23 5:19 ` sashiko-bot
2026-07-23 4:59 ` [PATCH v7 3/6] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers
` (3 subsequent siblings)
5 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-23 4:59 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, jolsa,
linux-kernel, linux-perf-users, mingo, peterz, tianyou.li
In add_script_opt(), when the browser selection has no valid hist_entry
(for example, when all entries are filtered out or the trace is empty),
hist_browser__selected_entry() returns NULL.
Add an explicit check for he == NULL prior to accessing he->time to avoid a
potential NULL pointer dereference crash.
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, 2 insertions(+)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index be8a6b169722..f5e13fc125c6 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2826,6 +2826,8 @@ add_script_opt(struct hist_browser *browser,
time_optstr++;
time_act++;
}
+ if (he == NULL)
+ return n;
j = sprintf(tstr, " in ");
j += timestamp__scnprintf_usec(he->time, tstr + j,
sizeof tstr - j);
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v7 2/6] perf ui hists: Guard against NULL hist_entry in add_script_opt()
2026-07-23 4:59 ` [PATCH v7 2/6] perf ui hists: Guard against NULL hist_entry in add_script_opt() Ian Rogers
@ 2026-07-23 5:19 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-23 5:19 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] NULL pointer dereference when toggling annotation with an empty browser selection.
--
--- Patch [2]: [PATCH v7 2/6] perf ui hists: Guard against NULL hist_entry in add_script_opt() ---
commit f95ba72ff3ba56daab82a876b00253253abac4da
Author: Ian Rogers <irogers@google.com>
perf ui hists: Guard against NULL hist_entry in add_script_opt()
This commit adds an explicit check for a NULL hist_entry in add_script_opt()
before accessing the entry's time field. This prevents a potential NULL
pointer dereference when the browser selection is empty or filtered out.
While reviewing this fix, I noticed a similar pre-existing issue in another
file.
tools/perf/builtin-c2c.c:perf_c2c__toggle_annotation()
> static int perf_c2c__toggle_annotation(struct hist_browser *browser)
> {
> struct hist_entry *he = browser->he_selection;
> // ...
> if (!perf_c2c__has_annotation(he->hists->hpp_list)) {
[Severity: High]
This is a pre-existing issue, but could this cause a segmentation fault if
the browser selection is empty? The code dereferences the he pointer to
access he->hists->hpp_list before validating it.
> ui_browser__help_window(&browser->b, "No annotation support");
> return 0;
> }
>
> if (he == NULL) {
If a user opens the c2c cacheline browser on an empty or completely
filtered dataset and presses the annotation hotkey, browser->he_selection
is NULL. Since the safety check happens after the dereference, it appears
this would crash the perf tool.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723045949.988740-1-irogers@google.com?part=2
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v7 3/6] perf ui hists: In report UI ensure thread is set with reference counting
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
2026-07-23 4:59 ` [PATCH v7 1/6] perf ui hists: Fix stack use-after-return in symbol_filter_str Ian Rogers
2026-07-23 4:59 ` [PATCH v7 2/6] perf ui hists: Guard against NULL hist_entry in add_script_opt() Ian Rogers
@ 2026-07-23 4:59 ` Ian Rogers
2026-07-23 4:59 ` [PATCH v7 4/6] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers
` (2 subsequent siblings)
5 siblings, 0 replies; 89+ messages in thread
From: Ian Rogers @ 2026-07-23 4:59 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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.
Fixes: 0e26ba5a8774 ("perf disasm: Refactor arch__find and initialization of arch structs")
Reported-by: Josh Stone <jistone@redhat.com>
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Ian Rogers <irogers@google.com>
---
tools/perf/ui/browsers/hists.c | 135 +++++++++++++++++++++------------
1 file changed, 87 insertions(+), 48 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index f5e13fc125c6..684df550ba49 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,16 @@ 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);
+ memset(&actions[i], 0, sizeof(struct popup_action));
+ }
+}
+
/*
* 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 +2474,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 +2551,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;
}
@@ -2573,7 +2583,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)
@@ -2628,7 +2638,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 +2685,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 +2730,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;
}
@@ -2734,8 +2744,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 +2753,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 +2810,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;
@@ -3097,6 +3107,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);
@@ -3156,24 +3168,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,
@@ -3183,18 +3211,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)
@@ -3209,12 +3239,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",
@@ -3232,9 +3266,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':
@@ -3306,20 +3342,19 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
continue;
}
- actions->ms.map = map;
+ memset(&hotkey_act, 0, sizeof(hotkey_act));
top = pstack__peek(browser->pstack);
if (top == &browser->hists->dso_filter) {
- /*
- * No need to set actions->dso here since
- * it's just to remove the current filter.
- */
- do_zoom_dso(browser, actions);
+ hotkey_act.ms.map = map__get(map);
+ do_zoom_dso(browser, &hotkey_act);
} else if (top == &browser->hists->thread_filter) {
- actions->thread = thread;
- do_zoom_thread(browser, actions);
+ hotkey_act.ms.thread = thread__get(thread);
+ do_zoom_thread(browser, &hotkey_act);
} else if (top == &browser->hists->socket_filter) {
- do_zoom_socket(browser, actions);
+ hotkey_act.socket = socked_id;
+ do_zoom_socket(browser, &hotkey_act);
}
+ map_symbol__exit(&hotkey_act.ms);
continue;
}
case 'q':
@@ -3456,9 +3491,13 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
if (key == K_SWITCH_INPUT_DATA)
break;
+
+ free_popup_options(options, MAX_OPTIONS);
+ free_popup_actions(actions, MAX_OPTIONS);
}
out_free_stack:
pstack__delete(browser->pstack);
+ free_popup_actions(actions, MAX_OPTIONS);
out:
hist_browser__delete(browser);
free_popup_options(options, MAX_OPTIONS);
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* [PATCH v7 4/6] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
` (2 preceding siblings ...)
2026-07-23 4:59 ` [PATCH v7 3/6] perf ui hists: In report UI ensure thread is set with reference counting Ian Rogers
@ 2026-07-23 4:59 ` Ian Rogers
2026-07-23 4:59 ` [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread Ian Rogers
2026-07-23 4:59 ` [PATCH v7 6/6] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
5 siblings, 0 replies; 89+ messages in thread
From: Ian Rogers @ 2026-07-23 4:59 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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, 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>
# Conflicts:
# tools/perf/ui/browsers/hists.c
---
tools/perf/ui/browsers/hists.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
index 684df550ba49..39d83f15f23c 100644
--- a/tools/perf/ui/browsers/hists.c
+++ b/tools/perf/ui/browsers/hists.c
@@ -2651,13 +2651,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);
}
@@ -3025,7 +3026,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];
@@ -3498,6 +3499,13 @@ 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;
+ perf_hpp__set_elide(HISTC_DSO, false);
+ perf_hpp__set_elide(HISTC_THREAD, false);
+ hists__filter_by_dso(hists);
+ hists__filter_by_thread(hists);
out:
hist_browser__delete(browser);
free_popup_options(options, MAX_OPTIONS);
--
2.55.0.229.g6434b31f56-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
` (3 preceding siblings ...)
2026-07-23 4:59 ` [PATCH v7 4/6] perf ui hists: Fix dso_filter reference leak and exit zoom cleanup Ian Rogers
@ 2026-07-23 4:59 ` Ian Rogers
2026-07-23 5:34 ` sashiko-bot
2026-07-23 4:59 ` [PATCH v7 6/6] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
5 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-23 4:59 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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 utilizing fallback paths such as
reading the e_machine from a map's DSO ELF header for user-space libraries.
Additionally, rely on map__kmaps() and maps__machine() to reliably extract
the recorded machine environment and e_machine for kernel and kallsyms maps,
perfectly preventing silent, incorrect host fallbacks to uname() during
cross-platform Capstone annotation sessions.
At the same time, ensure all remaining uses of a map_symbol's thread pointer
do not assume it is non-NULL to eliminate UI segmentation faults, and remove
the fragile, redundant thread__get_arch() function to streamline the
annotate and disassembly subsystem architecture.
Fixes: 0e26ba5a8774 ("perf disasm: Refactor arch__find and initialization of arch structs")
Assisted-by: Antigravity:gemini-3.5-flash
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 | 3 +-
tools/perf/util/capstone.c | 42 ++++++++++++++++++++-----
4 files changed, 74 insertions(+), 24 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..df70e95a8470 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -982,24 +982,43 @@ 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;
+ const char *cpuid = NULL;
+
+ if (ms->thread) {
+ machine = maps__machine(thread__maps(ms->thread));
+ e_machine = thread__e_machine(ms->thread, machine, &e_flags);
+ if (machine && machine->env)
+ cpuid = machine->env->cpuid;
+ } else if (dso) {
+ struct maps *kmaps = (map && dso__kernel(dso)) ? map__kmaps(map) : NULL;
+ struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
+
+ e_machine = dso__e_machine(dso, kmap_machine, &e_flags);
+ if (kmap_machine && kmap_machine->env)
+ cpuid = kmap_machine->env->cpuid;
}
- 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, cpuid);
if (arch == NULL) {
pr_err("%s: unsupported arch %d\n", __func__, e_machine);
- return errno;
+ /* TODO: Refactor annotate/disassemble subsystem error
+ * codes to uniformly return negative integers.
+ */
+ return errno ? errno : ENOTSUP;
}
if (parch)
*parch = arch;
@@ -1018,7 +1037,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 +1270,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 +1290,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 +1394,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 00e0141cae8d..74213daf8786 100644
--- a/tools/perf/util/capstone.c
+++ b/tools/perf/util/capstone.c
@@ -392,7 +392,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)
@@ -423,9 +423,22 @@ 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) {
+ struct maps *kmaps = (map && dso__kernel(dso)) ? map__kmaps(map) : NULL;
+ struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
+
+ e_machine = dso__e_machine_endian(dso, kmap_machine, /*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;
@@ -518,7 +531,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)
@@ -538,9 +551,22 @@ 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) {
+ struct maps *kmaps = (map && dso__kernel(dso)) ? map__kmaps(map) : NULL;
+ struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
+
+ e_machine = dso__e_machine_endian(dso, kmap_machine, /*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.229.g6434b31f56-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread
2026-07-23 4:59 ` [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread Ian Rogers
@ 2026-07-23 5:34 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-23 5:34 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The patch actively introduces a silent fallback to the host architecture for userspace maps, defeating its stated purpose.
- [Medium] Returning a positive error code (ENOTSUP) from hist_entry__annotate_printf causes the caller to misinterpret the error as a valid count of filtered lines.
--
--- Patch [5]: [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread ---
commit fcda5c254c589dfc6e3daa2b89d76183be5ba280
Author: Ian Rogers <irogers@google.com>
perf annotate: Be robust to annotating without a thread
This commit attempts to improve architecture detection for map symbols when
a thread isn't provided. It implements fallbacks using the DSO ELF header
and kmaps to avoid incorrectly falling back to the host architecture during
cross-platform analysis.
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 53b2a224b21df..df70e95a84704 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
[ ... ]
> @@ -982,24 +982,43 @@ void symbol__calc_percent(struct symbol *sym, struct evsel *evsel)
[ ... ]
> + if (ms->thread) {
> + machine = maps__machine(thread__maps(ms->thread));
> + e_machine = thread__e_machine(ms->thread, machine, &e_flags);
> + if (machine && machine->env)
> + cpuid = machine->env->cpuid;
> + } else if (dso) {
> + struct maps *kmaps = (map && dso__kernel(dso)) ? map__kmaps(map) : NULL;
> + struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> + e_machine = dso__e_machine(dso, kmap_machine, &e_flags);
> + if (kmap_machine && kmap_machine->env)
> + cpuid = kmap_machine->env->cpuid;
> }
>
> - 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);
[Severity: High]
Does this code silently fall back to the host architecture, defeating the stated
purpose of the patch? The commit message mentions "perfectly preventing silent,
incorrect host fallbacks to uname()", but when a user-space DSO is missing or
uses JIT, dso__e_machine() might return EM_NONE. Passing NULL to
thread__e_machine() triggers the uname() fallback, which assigns EM_HOST and
could cause Capstone to disassemble using the wrong architecture instead of
correctly aborting.
> +
> + arch = arch__find(e_machine, e_flags, cpuid);
> if (arch == NULL) {
[ ... ]
> @@ -1251,6 +1270,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;
[Severity: Medium]
Will returning a positive error code (ENOTSUP) here cause the caller to
misinterpret the error as a valid count of filtered lines?
perf_top__show_details() interprets any positive return value from
hist_entry__annotate_printf() as the number of lines filtered out, incorrectly
printing '95 lines not displayed' rather than handling the failure. Should this
return a negative error code like -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" :
> diff --git a/tools/perf/util/capstone.c b/tools/perf/util/capstone.c
> index 00e0141cae8db..74213daf87862 100644
> --- a/tools/perf/util/capstone.c
> +++ b/tools/perf/util/capstone.c
[ ... ]
> @@ -423,9 +423,22 @@ 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) {
> + struct maps *kmaps = (map && dso__kernel(dso)) ? map__kmaps(map) : NULL;
> + struct machine *kmap_machine = kmaps ? maps__machine(kmaps) : NULL;
> +
> + e_machine = dso__e_machine_endian(dso, kmap_machine, /*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);
[Severity: High]
Does this code also silently fall back to the host architecture here? Similar to
map_symbol__get_arch(), passing NULL to thread__e_machine_endian() results in
using the host architecture instead of aborting properly when annotating
cross-platform perf.data files.
> + }
> if (capstone_init(e_machine, &handle, is_64bit, is_big_endian, disassembler_style) < 0)
> goto err;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723045949.988740-1-irogers@google.com?part=5
^ permalink raw reply [flat|nested] 89+ messages in thread
* [PATCH v7 6/6] perf ui hists: Remove duplicated thread in popup_action
2026-07-23 4:59 ` [PATCH v7 0/6] perf ui hists / annotate: Remaining fixes for reference counting and annotation Ian Rogers
` (4 preceding siblings ...)
2026-07-23 4:59 ` [PATCH v7 5/6] perf annotate: Be robust to annotating without a thread Ian Rogers
@ 2026-07-23 4:59 ` Ian Rogers
2026-07-23 5:32 ` sashiko-bot
5 siblings, 1 reply; 89+ messages in thread
From: Ian Rogers @ 2026-07-23 4:59 UTC (permalink / raw)
To: irogers, acme, namhyung
Cc: adrian.hunter, dapeng1.mi, james.clark, jistone, 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 39d83f15f23c..c15874a491b4 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.229.g6434b31f56-goog
^ permalink raw reply related [flat|nested] 89+ messages in thread* Re: [PATCH v7 6/6] perf ui hists: Remove duplicated thread in popup_action
2026-07-23 4:59 ` [PATCH v7 6/6] perf ui hists: Remove duplicated thread in popup_action Ian Rogers
@ 2026-07-23 5:32 ` sashiko-bot
0 siblings, 0 replies; 89+ messages in thread
From: sashiko-bot @ 2026-07-23 5:32 UTC (permalink / raw)
To: Ian Rogers; +Cc: linux-perf-users
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The symbol filter string is not reset and freed when exiting the browser, causing silent filter persistence and a memory leak.
--
--- Patch [6]: [PATCH v7 6/6] perf ui hists: Remove duplicated thread in popup_action ---
commit 0930d22bfe566125dece911349b0d1a0a381a8df
Author: Ian Rogers <irogers@google.com>
perf ui hists: Remove duplicated thread in popup_action
struct popup_action has a thread but this is duplicated in the
map_symbol. Remove the non-map_symbol version so that there's only ever
1 thread with a popup_action.
> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 39d83f15f23c6..c15874a491b43 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;
tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
...
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;
perf_hpp__set_elide(HISTC_DSO, false);
perf_hpp__set_elide(HISTC_THREAD, false);
hists__filter_by_dso(hists);
hists__filter_by_thread(hists);
out:
...
}
[Severity: Medium]
Does this exit cleanup path leak the symbol filter string?
It appears the dso and thread filters are properly reset here, but
hists->symbol_filter_str is completely omitted from the cleanup.
Could this result in a memory leak and silent filter persistence state? If a
user applies a symbol filter via the '/' hotkey and then exits the view,
the view might become silently filtered without UI indication in the stack
when switching browser tabs.
The commit message for 'perf ui hists: Fix stack use-after-return in
symbol_filter_str' explicitly promised to 'cleanly reset and free the symbol
filter string upon exiting the browser', but does this code fail to do so?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260723045949.988740-1-irogers@google.com?part=6
^ permalink raw reply [flat|nested] 89+ messages in thread