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

[sashiko-bot@kernel.org]

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