Re: [PATCH v5 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] Unchecked dereference of `he->time` when no histogram entry is selected and `sort_order` contains 'time'.
- [Medium] Memory leak of dynamically allocated menu option strings across iterations of the UI event loop.
--

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

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

This commit fixes a crash in ui__popup_menu() caused by an unconditional
pointer increment for an optional 'time' popup action. It correctly
advances the optstr and act pointers only if the first script addition
succeeds.

> 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, "");
> @@ -2819,17 +2819,24 @@ add_script_opt(struct hist_browser *browser,
>  	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 this lead to a NULL pointer dereference
if the histogram browser has no selected entry?

If a user interacts with an empty view and sort_order contains "time",
evsel__hists_browse() will call add_script_opt() when no entry is selected.
The call to hist_browser__selected_entry(browser) above will return NULL,
causing a crash when he->time is accessed here.

>  		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);

[Severity: Medium]
This is a pre-existing issue, but does this leak memory when the popup menu
is repeatedly opened and closed?

The add_script_opt_2() function dynamically allocates strings into the options
array using asprintf(). In evsel__hists_browse(), the main event loop resets
the option count:

tools/perf/ui/browsers/hists.c:evsel__hists_browse() {
    ...
    choice = nr_options = 0;
    ...
}

Subsequent calls to functions like add_script_opt_2() will then overwrite
these string pointers in the options array without freeing the previously
allocated strings.

> +		if (ret > 0) {
> +			time_act->time = he->time;
> +			n += ret;
> +		}
>  	}
>  	return n;
>  }

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