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