Re: [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Memory leak of dynamically allocated popup menu strings in the interaction loop.
- [Medium] Missing reference counting for `dso_filter` can lead to a use-after-free.
- [Medium] Unconditional pointer increment after string allocation failure leads to a NULL pointer dereference.
--

--- Patch [4]: [PATCH v3 4/4] perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow ---
commit b8ca9e63522e300d76cedfda674dbc631afbc3e2
Author: Ian Rogers <irogers@google.com>

perf hists browser: Increase MAX_OPTIONS to prevent stack buffer overflow

The commit increases MAX_OPTIONS in evsel__hists_browse() from 16 to 32 to
prevent a potential stack buffer overflow when adding menu options. It also
moves the memset of options and actions earlier in the function to avoid
reading uninitialized memory on an error path.

Closes: https://lore.kernel.org/linux-perf-users/20260708235834.3FB771F00A3A@smtp.kernel.org/
Fixes: f2b487db45f2 ("perf hists browser: Fix possible memory leak")

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index a93846b8dc885..8e54c29728d1c 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3002,7 +3002,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  	struct hists *hists = evsel__hists(evsel);
>  	struct hist_browser *browser = perf_evsel_browser__new(evsel, hbt, env);
>  	struct branch_info *bi = NULL;
> -#define MAX_OPTIONS  16
> +#define MAX_OPTIONS  32
>  	char *options[MAX_OPTIONS];
>  	struct popup_action actions[MAX_OPTIONS];
>  	int nr_options = 0;
> @@ -3063,15 +3063,15 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  		browser->min_pcnt = min_pcnt;
>  	hist_browser__update_nr_entries(browser);
>  
> +	memset(options, 0, sizeof(options));
> +	memset(actions, 0, sizeof(actions));
> +
>  	browser->pstack = pstack__new(3);
>  	if (browser->pstack == NULL)
>  		goto out;
>  
>  	ui_helpline__push(helpline);
>  
> -	memset(options, 0, sizeof(options));
> -	memset(actions, 0, sizeof(actions));
> -
>  	if (symbol_conf.col_width_list_str)
>  		perf_hpp__set_user_width(symbol_conf.col_width_list_str);
>  

[Severity: Medium]
This is a pre-existing issue, but I noticed a potential memory leak in
evsel__hists_browse() later in the same function when handling the options
array.

When the interactive loop runs, the dynamically allocated strings added to
the options array via asprintf() appear to be abandoned because nr_options
is reset to 0 in each iteration without freeing them:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
    while (1) {
        ...
        key = 0; // reset key
do_hotkey:       // key came straight from options ui__popup_menu()
        choice = nr_options = 0;
    ...
}

Does this cause memory to leak continuously as the user interacts with the
menu and new strings are allocated?


[Severity: Medium]
This is another pre-existing issue, but I noticed a potential missing
reference count in hists_browser__zoom_map().

The reference-counted dso struct is assigned as a raw pointer without
acquiring a reference via dso__get():

tools/perf/ui/browsers/hists.c:hists_browser__zoom_map() {
    ...
    browser->hists->dso_filter = dso;
    ...
}

Could this lead to a use-after-free defect if the underlying dso is freed
while the filter remains active, since it's later cleared without calling
dso__put()?


[Severity: Medium]
This is also a pre-existing issue, but I found a sequence in add_script_opt()
that might cause a NULL pointer dereference.

If the first call to add_script_opt_2() fails (for example, due to an
asprintf allocation failure), the pointer remains unset. The function then
unconditionally increments the optstr and act pointers:

tools/perf/ui/browsers/hists.c:add_script_opt() {
    ...
    optstr++;
    act++;
    ...
}

If a second allocation then completes successfully, the caller increments
nr_options, but the options array would contain an uninitialized or NULL
pointer gap at its first active index.

Can this NULL pointer be subsequently dereferenced by ui__popup_menu() via
ui_browser__write_nstring()?

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