Re: [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] Memory leak of dynamically allocated strings for the popup menu in `evsel__hists_browse`.
- [Medium] NULL pointer dereference when computing script option time range for an empty trace.
--

--- Patch [6]: [PATCH v6 06/10] perf ui hists: Fix NULL pointer array gap in add_script_opt() ---
commit 7d70d8b9859cd9c3627530a34fb514614a33b89f
Author: Ian Rogers <irogers@google.com>

perf ui hists: Fix NULL pointer array gap in add_script_opt()

In add_script_opt(), the function unconditionally increments the optstr
and act pointers for a second optional 'time' popup action before
attempting to invoke add_script_opt_2(). If the first add_script_opt_2()
call failed, this leaves a NULL pointer gap in the options array. When
ui__popup_menu() is later displayed, it dereferences this gap and crashes.

> diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> index 4282ef1b81f75..46fb484a4dc64 100644
> --- a/tools/perf/ui/browsers/hists.c
> +++ b/tools/perf/ui/browsers/hists.c
> @@ -2811,7 +2811,7 @@ add_script_opt(struct hist_browser *browser,
>  	       struct popup_action *act, char **optstr,
>  	       struct thread *thread, struct symbol *sym)
>  {
> -	int n, j;
> +	int n, j, ret;
>  	struct hist_entry *he;
>  
>  	n = add_script_opt_2(act, optstr, thread, sym, "");

[Severity: Medium]
This is a pre-existing issue, but does this dynamically allocated string leak?

When repeatedly opening and closing the popup menu (pressing 'm' then 'ESC'),
evsel__hists_browse() resets nr_options to 0 inside the event loop.

This overwrites the options array without calling free_popup_options() to clean
up the strings allocated by asprintf() in add_script_opt_2().

>  
>  	he = hist_browser__selected_entry(browser);
>  	if (sort_order && strstr(sort_order, "time")) {
>  		char tstr[128];
> +		struct popup_action *time_act = act;
> +		char **time_optstr = optstr;
>  
> -		optstr++;
> -		act++;
> +		if (n > 0) {
> +			time_optstr++;
> +			time_act++;
> +		}
>  		j = sprintf(tstr, " in ");
>  		j += timestamp__scnprintf_usec(he->time, tstr + j,
>  					       sizeof tstr - j);

[Severity: Medium]
This is a pre-existing issue, but can he be NULL here leading to a crash?

If the trace is empty (e.g., all entries filtered out) and the user presses 'm',
evsel__hists_browse() calls add_script_opt() unconditionally, even if
hist_browser__selected_entry() returns NULL.

>  		j += sprintf(tstr + j, "-");
>  		timestamp__scnprintf_usec(he->time + symbol_conf.time_quantum,
>  				          tstr + j, sizeof tstr - j);
> -		n += add_script_opt_2(act, optstr, thread, sym, tstr);
> -		act->time = he->time;
> +		ret = add_script_opt_2(time_act, time_optstr, thread, sym, tstr);
> +		if (ret > 0) {
> +			time_act->time = he->time;
> +			n += ret;
> +		}
>  	}
>  	return n;
>  }

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