Re: [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Stack buffer overflow in `evsel__hists_browse()` due to unbounded popup menu options
--

--- Patch [1]: [PATCH v3 1/4] perf ui hists: In report UI ensure thread is set ---
commit 6b0cc8c7dd2c933b2c9f1bf775218027677c766a
Author: Ian Rogers <irogers@google.com>

perf ui hists: In report UI ensure thread is set

This commit populates the thread field in map_symbol within the hists browser
UI to fix cross-platform disassembly. It intentionally uses raw pointers for
thread and map to avoid reference counting issues tied to the bounded lifecycle
of the actions array.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3155,10 +3155,12 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  						continue;
>  
>  					actions->ms.sym = symbol__new_unresolved(bi->to.al_addr, bi->to.ms.map);
> +					actions->ms.thread = bi->to.ms.thread;
>  					actions->ms.map = bi->to.ms.map;
>  				} else {
>  					actions->ms.sym = symbol__new_unresolved(browser->he_selection->ip,
>  										 browser->selection->map);
> +					actions->ms.thread = browser->selection->thread;
>  					actions->ms.map = browser->selection->map;
>  				}
>  
> @@ -3173,6 +3175,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					continue;
>  				}
>  
> +				actions->ms.thread = browser->selection->thread;

[Severity: Critical]
This is a pre-existing issue, but does the unbounded accumulation of options
risk overflowing the actions[] and options[] arrays on the stack?

Looking at evsel__hists_browse(), the arrays are fixed to MAX_OPTIONS (16):

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
#define MAX_OPTIONS  16
    char *options[MAX_OPTIONS];
    struct popup_action actions[MAX_OPTIONS];
    int nr_options = 0;

Later in the function when building the popup menu, there is a long sequence
of add_*_opt() calls without any bounds checking on nr_options:

    nr_options += add_annotate_opt(&actions[nr_options], ...);
    ...
    nr_options += add_annotate_type_opt(&actions[nr_options], ...);
    nr_options += add_thread_opt(browser, &actions[nr_options], ...);
    nr_options += add_dso_opt(browser, &actions[nr_options], ...);
    nr_options += add_callchain_toggle_opt(browser, &actions[nr_options], ...);
    nr_options += add_map_opt(browser, &actions[nr_options], ...);
    nr_options += add_socket_opt(browser, &actions[nr_options], ...);

If enough of these conditions are met simultaneously (e.g. branch annotations,
type, thread, DSO, callchain, map, socket, and multiple scripts), couldn't
nr_options exceed 16 and corrupt the stack?

>  				actions->ms.map = browser->selection->map;
>  				actions->ms.sym = browser->selection->sym;
>  			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709165220.1411149-1-irogers@google.com?part=1