Re: [PATCH v6 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] 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