Re: [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

[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 options during main interaction loop.
- [Medium] Unconditional pointer increments leave uninitialized or undefined array slots on allocation failure.
- [Low] Missing explicit POSIX header inclusions breaking musl libc compatibility.
--

--- Patch [2]: [PATCH v5 02/10] perf ui hists: Fix uninitialized stack memory free on pstack allocation failure ---
commit 77f870443b8c814782369da867fcd967bbc78bdf
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix uninitialized stack memory free on pstack allocation failure

Fixes heap corruption by initializing the options and actions arrays before
the pstack allocation check, preventing an uninitialized stack pointer from
being passed to free_popup_options() if the allocation fails.

Link: https://lore.kernel.org/linux-perf-users/20260709035230.6DBEE1F000E9@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 671961946d56c..45958bbe45092 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3064,15 +3064,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 does this code leak dynamically allocated
popup menu options during the main interaction loop?

Looking at evsel__hists_browse() when returning to the do_hotkey label:

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

It appears nr_options is reset to 0 without freeing the strings previously
allocated and stored in the options array. The subsequent iteration might
overwrite the string pointers, permanently leaking the previously allocated
strings. Could we ensure the strings are freed here?

[Severity: Low]
This is a pre-existing issue, but could missing explicit POSIX header
inclusions break musl libc compatibility?

For example, hist_browser__dump() uses access() and F_OK without explicitly
including <unistd.h>:

tools/perf/ui/browsers/hists.c:hist_browser__dump() {
    ...
    if (access(filename, F_OK))
    ...
}

Similarly, switch_data_file() relies on PATH_MAX without explicitly including
<limits.h>:

tools/perf/ui/browsers/hists.c:switch_data_file() {
    ...
    char path[PATH_MAX];
    ...
}

Could these headers be explicitly included to ensure musl libc builds don't
fail?

[Severity: Medium]
This is a pre-existing issue, but do unconditional pointer increments here
leave uninitialized array slots on allocation failure?

Looking at add_script_opt():

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

If a previous call to add_script_opt_2() fails (e.g., asprintf fails and
returns 0), the target string pointer is left unpopulated. However, this
code unconditionally increments the array pointers. If a subsequent allocation
succeeds, the array gets populated with a gap, but nr_options is still
incremented.

Might this cause the UI menu iteration to dereference an undefined or NULL
pointer at the gap?

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