Re: [PATCH 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unconditional memory leak of dynamically allocated format structures in `print_pareto()`.
- [High] Architectural/API Misuse: `perf_hpp__setup_output_field()` and `perf_hpp__append_sort_keys()` silently corrupt the global `perf_hpp_list`.
--

--- Patch [2]: [PATCH 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure ---
commit c2ba37f5e0548f4ffed858d39e640349044131b2
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf c2c: Clean up registered formats on c2c_hists__init() failure

This commit addresses a memory leak when c2c_hists__init() fails partway
through parsing. It ensures partially registered format structures are
unregistered before returning the error. This protects callers from leaking
memory during report generation.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index b0f2ba8318304..e66e9ca8618dc 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -2142,6 +2142,8 @@ static int c2c_hists__init(struct c2c_hists *hists,
>  			   int nr_header_lines,
>  			   struct perf_env *env)
>  {
> +	int ret;
> +
>  	__hists__init(&hists->hists, &hists->list);
>  
>  	/*
> @@ -2154,7 +2156,13 @@ static int c2c_hists__init(struct c2c_hists *hists,
>  	/* Overload number of header lines.*/
>  	hists->list.nr_header_lines = nr_header_lines;
>  
> -	return hpp_list__parse(&hists->list, NULL, sort, env);
> +	ret = hpp_list__parse(&hists->list, NULL, sort, env);
> +
> +	/* Unregister any formats added before the failure point */
> +	if (ret)
> +		perf_hpp__reset_output_field(&hists->list);
> +
> +	return ret;
>  }

[Severity: High]
This is a pre-existing issue, but does print_pareto() unconditionally leak
dynamically allocated format structures?

Every time print_pareto() is called during C2C reporting, it initializes
a local stack variable for the formats:

tools/perf/builtin-c2c.c:print_pareto() {
        struct perf_hpp_list hpp_list;
        ...
        perf_hpp_list__init(&hpp_list);
        ret = hpp_list__parse(&hpp_list, cl_output, NULL, env);
        ...
}

However, when the function exits on both success and error paths, it doesn't
call perf_hpp__reset_output_field() on hpp_list. Does this mean the formats
attached to this local list are permanently leaked when the list goes out of
scope?

[Severity: High]
This is a pre-existing issue, but do perf_hpp__setup_output_field() and
perf_hpp__append_sort_keys() unintentionally corrupt the global
perf_hpp_list instead of updating the passed local list?

When these functions are called with a local list, they iterate over the
local list but invoke the register functions directly:

tools/perf/ui/hist.c:perf_hpp__setup_output_field() {
        ...
        perf_hpp__column_register(fmt);
        ...
}

tools/perf/ui/hist.c:perf_hpp__append_sort_keys() {
        ...
        perf_hpp__register_sort_field(fmt);
        ...
}

These wrappers appear to be hardcoded to insert elements into the global list:

tools/perf/util/hist.h:
#define perf_hpp__column_register(format) \
        perf_hpp_list__column_register(&perf_hpp_list, format)

Does this leave the required fields missing from the local list, while
simultaneously polluting the global perf_hpp_list state with dynamically
allocated objects?

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