Re: [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Raw pointer assignment of reference-counted structs violates REFCNT_CHECKING lifecycle rules.

Pre-existing issues:
- [High] Stack-allocated buffer `buf` is assigned to a long-lived structure field `hists->symbol_filter_str`, leading to a Use-After-Free when the function returns.
--

--- Patch [5]: [PATCH v4 5/9] perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop ---
commit 8a51dcaa31cd8e1cae7b31630b60408cb040d87b
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix memory leak in evsel__hists_browse() interactive loop

In the evsel__hists_browse() interactive loop, the options array is
filled with strings dynamically allocated via asprintf() and displayed
in a popup menu. However, if the user picks a hotkey or triggers another
menu iteration, nr_options is reset to 0 without freeing the prior
strings, leaving them abandoned in the array and leaking memory on every
menu event.

Fix it by invoking free_popup_options(options, MAX_OPTIONS) at the
beginning of each menu iteration (covering both the while(1) start and
the 'do_hotkey:' label).

Closes: https://lore.kernel.org/linux-perf-users/20260709170834.52F1A1F000E9@smtp.kernel.org/

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 8b4c7510991f3..aed154622467f 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -3087,6 +3087,7 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  
>  		key = 0; // reset key
>  do_hotkey:		 // key came straight from options ui__popup_menu()
> +		free_popup_options(options, MAX_OPTIONS);
>  		choice = nr_options = 0;
>  		key = hist_browser__run(browser, helpline, warn_lost_event, key);
>  

[ ... ]

> @@ -3156,6 +3157,6 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  					if (!bi || !bi->to.ms.map)
>  						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;

[Severity: Medium]
In tools/perf/ui/browsers/hists.c:evsel__hists_browse(), is it safe to
directly assign actions->ms.thread without using thread__get() when
processing the SORT_MODE__BRANCH selection?

It seems this raw pointer assignment of a reference-counted struct violates
lifecycle rules. Could this lead to asan use-after-free traps or lsan
leaks during the interactive menu loop?

[ ... ]

> @@ -3215,6 +3216,6 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
>  		case '/':
>  			if (ui_browser__input_window("Symbol to show",
>  					"Please enter the name of symbol you want to see.\n"
>  					"To remove the filter later, press / + ENTER.",
>  					buf, "ENTER: OK, ESC: Cancel",
>  					delay_secs * 2) == K_ENTER) {
>  				hists->symbol_filter_str = *buf ? buf : NULL;
>  				hists__filter_by_symbol(hists);

[Severity: High]
This is a pre-existing issue, but in tools/perf/ui/browsers/hists.c:evsel__hists_browse(),
doesn't this assign a stack-allocated buffer (buf) to the persistent
hists->symbol_filter_str pointer without duplicating it?

If a user applies a symbol filter in the single-event browser view and then
exits, could this cause a use-after-free when evsel__hists_browse() returns
while a background thread is still applying filters?

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