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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] perf_hpp__setup_output_field() and perf_hpp__append_sort_keys() corrupt the global perf_hpp_list and fail to populate the provided local list.
--

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

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

When c2c_hists__init() calls hpp_list__parse() and it fails partway through,
format structures registered are left on the hpp_list. This fix adds cleanup
inside c2c_hists__init() itself so all callers are protected.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 160b82694d391..a01fa1655569c 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c

[ ... ]

> @@ -2159,7 +2161,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, /*output=*/NULL, sort, env);
> +	ret = hpp_list__parse(&hists->list, /*output=*/NULL, sort, env);

[Severity: High]
This isn't a bug introduced by this patch, but will calling hpp_list__parse()
corrupt the global perf_hpp_list?

When c2c initializes its local hpp_list, hpp_list__parse() eventually leads
to calls to perf_hpp__setup_output_field() and perf_hpp__append_sort_keys()
in tools/perf/ui/hist.c. Both of these functions accept a local
struct perf_hpp_list *list parameter, but they appear to use the global list
wrappers instead of modifying the passed local list:

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

Could this cause the local lists to miss output columns, while also polluting
the global list with dangling pointers to dynamically allocated local formats?

Should these functions be updated to call
perf_hpp_list__column_register(list, fmt) and
perf_hpp_list__register_sort_field(list, fmt) instead?

> +
> +	/* Unregister any formats added before the failure point */
> +	if (ret)
> +		perf_hpp__reset_output_field(&hists->list);
> +
> +	return ret;
>  }
>

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