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

[Namhyung Kim <namhyung@kernel.org>]

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