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

[sashiko-bot@kernel.org]

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