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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Medium] print_pareto() calls hpp_list__parse() on a local stack variable but never cleans it up, leaking dynamically allocated formats.
- [Medium] kvm_hists__init() and kvm_hists__reinit() suffer from identical partial-registration leaks when format parsing fails.
--

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

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

When c2c_hists__init() or c2c_hists__reinit() calls hpp_list__parse()
and it fails partway through, format structures registered are left on
the hpp_list. This commit adds cleanup inside both functions to protect
all callers from leaking partially registered entries.

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

[ ... ]

> @@ -2167,8 +2175,16 @@ static int c2c_hists__reinit(struct c2c_hists *c2c_hists,
>  			     const char *sort,
>  			     struct perf_env *env)
>  {
> +	int ret;
> +
>  	perf_hpp__reset_output_field(&c2c_hists->list);
> -	return hpp_list__parse(&c2c_hists->list, output, sort, env);
> +	ret = hpp_list__parse(&c2c_hists->list, output, sort, env);
> +
> +	/* Unregister any formats added before the failure point */
> +	if (ret)
> +		perf_hpp__reset_output_field(&c2c_hists->list);
> +
> +	return ret;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed that print_pareto()
in the same file calls hpp_list__parse() on a local stack variable:

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

Because print_pareto() returns without ever calling 
perf_hpp__reset_output_field(), does it leak all allocated c2c_fmt structures 
on both the success and failure paths?

[Severity: Medium]
This is a pre-existing issue, but kvm_hists__init() and kvm_hists__reinit() in
tools/perf/builtin-kvm.c seem to suffer from identical partial-registration
leaks when format parsing fails:

tools/perf/builtin-kvm.c:kvm_hists__init() {
    ...
    return kvm_hpp_list__parse(&kvm_hists.list, NULL, "ev_name");
}

tools/perf/builtin-kvm.c:kvm_hists__reinit() {
    ...
    return kvm_hpp_list__parse(&kvm_hists.list, output, sort);
}

If parsing fails partway through these functions, will partially allocated
formats remain in the global list without cleanup, replicating the exact leak
pattern fixed here in c2c?

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