Re: [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str

[Namhyung Kim <namhyung@kernel.org>]

On Thu, Jul 16, 2026 at 07:48:47AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Unconditional free of a non-heap `argv` pointer leading to a crash, and unintended destruction of the CLI symbol filter state.
> --
> 
> --- Patch [4]: [PATCH v6 04/10] perf ui hists: Fix stack use-after-return in symbol_filter_str ---
> commit 7f822e6fd8f11b66dda82bea74a7a54f3a875831
> Author: Ian Rogers <irogers@google.com>
> 
> perf ui hists: Fix stack use-after-return in symbol_filter_str
> 
> In evsel__hists_browse(), the local stack array 'buf' is assigned directly
> to the persistent 'hists->symbol_filter_str' pointer. This commit fixes the
> dangling pointer by duplicating the string and cleanly freeing it on exit.
> 
> > diff --git a/tools/perf/ui/browsers/hists.c b/tools/perf/ui/browsers/hists.c
> > index 793959574a9ad..4282ef1b81f75 100644
> > --- a/tools/perf/ui/browsers/hists.c
> > +++ b/tools/perf/ui/browsers/hists.c
> > @@ -3213,7 +3213,10 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> >  					"To remove the filter later, press / + ENTER.",
> >  					buf, "ENTER: OK, ESC: Cancel",
> >  					delay_secs * 2) == K_ENTER) {
> > -				hists->symbol_filter_str = *buf ? buf : NULL;
> > +				char *new_filter = *buf ? strdup(buf) : NULL;
> > +
> > +				zfree(&hists->symbol_filter_str);
> 
> [Severity: High]
> If the user invoked perf report with a command line option like
> --symbol-filter=my_func, hists->symbol_filter_str will have been assigned
> directly to the argv pointer in tools/perf/builtin-report.c:
> 
> report__collapse_hists() {
>     ...
>     hists->symbol_filter_str = rep->symbol_filter_str;
> 
> Will calling zfree() here cause a glibc invalid pointer crash when the
> user interactively applies a new filter?

Right, please update there to copy the command line string.  It seems
there's a test code to set it too.

Thanks,
Namhyung

> 
> > +				hists->symbol_filter_str = new_filter;
> >  				hists__filter_by_symbol(hists);
> >  				hist_browser__reset(browser);
> >  			}
> 
> [ ... ]
> 
> > @@ -3447,6 +3450,8 @@ static int evsel__hists_browse(struct evsel *evsel, int nr_events, const char *h
> >  	}
> >  out_free_stack:
> >  	pstack__delete(browser->pstack);
> > +	zfree(&hists->symbol_filter_str);
> > +	hists__filter_by_symbol(hists);
> 
> [Severity: High]
> Similarly, does this unconditional free on exit crash if the filter was
> initialized from the CLI?
> 
> Additionally, does unconditionally clearing this pointer permanently destroy
> the --symbol-filter state for subsequent views of the same event?
> 
> >  out:
> >  	hist_browser__delete(browser);
> >  	free_popup_options(options, MAX_OPTIONS);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260716072351.560311-1-irogers@google.com?part=4