Re: [PATCH 6/9] perf c2c: Free format list entries when c2c_hists__init() fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `perf_hpp__setup_output_field()` mistakenly appends format elements to the global `perf_hpp_list.fields` instead of the local `list->fields`, leading to an unbounded accumulation of `c2c_fmt` structures.
- [Medium] `hpp_list__parse()` and the `PARSE_LIST` macro silently overwrite and drop error return codes during format parsing.
--

commit f3900597f4c802568083103a6a640b3c7cb043fa
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf c2c: Free format list entries when c2c_hists__init() fails

This commit fixes a memory leak in the error path of he__get_c2c_hists().
It calls perf_hpp__reset_output_field() before freeing the hists container
when c2c_hists__init() fails, ensuring dynamically allocated format entries
are properly unregistered.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -226,6 +226,7 @@ he__get_c2c_hists(struct hist_entry *he,
>  
>  	ret = c2c_hists__init(hists, sort, nr_header_lines, env);

[Severity: Medium]
This isn't a bug introduced by this patch, but does hpp_list__parse()
(called within c2c_hists__init()) silently drop error return codes?

Looking at the PARSE_LIST macro in tools/perf/builtin-c2c.c, it initializes
ret = 0 internally on every invocation:

#define PARSE_LIST(_list, _fn) \
    do { \
        char *tmp, *tok; \
        ret = 0;

In hpp_list__parse(), sequential macro invocations mean the first error
code can be discarded:

    PARSE_LIST(output, c2c_hists__init_output);
    PARSE_LIST(sort,   c2c_hists__init_sort);

If the output parsing encounters an error, it breaks out and correctly sets
ret, but the subsequent sort parsing invocation immediately resets ret = 0.
Additionally, if strdup fails, it breaks but leaves ret = 0.

Could this cause allocation or parsing failures during c2c_hists__init()
to be silently ignored, returning success despite missing formatting?

[Severity: High]
This is a pre-existing issue, but is there an unbounded accumulation of
c2c_fmt structures occurring when he__get_c2c_hists() initializes
per-cacheline history lists?

When c2c_hists__init() sets up the local list, it eventually calls
perf_hpp__setup_output_field(). This function takes a local list parameter
but uses an inline wrapper that hardcodes the global list:

tools/perf/util/hist.h:perf_hpp__column_register() {
    perf_hpp_list__column_register(&perf_hpp_list, format);
}

Because perf_hpp__setup_output_field() in tools/perf/ui/hist.c uses this
global wrapper (perf_hpp__column_register(fmt)) instead of the local
variant, the format elements intended for each cacheline's local history
get appended to the global perf_hpp_list.fields.

Since perf c2c can process millions of cachelines, could this cause the
global list to endlessly accumulate dynamically allocated format elements?

>  	if (ret) {
> +		perf_hpp__reset_output_field(&hists->list);
>  		c2c_he->hists = NULL;
>  		free(hists);
>  		return NULL;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615213657.35776-1-acme@kernel.org?part=6