Re: [PATCH 13/15] perf c2c: Free format list entries when releasing c2c hist entries

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Memory leak of inner `hist_entry` objects in `entries_in_array` when releasing outer cacheline entries in `c2c_he_free()`.
- [Medium] Memory leak of dynamically allocated format list entries on the error path in `he__get_c2c_hists()`.
--

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

perf c2c: Free format list entries when releasing c2c hist entries

This commit fixes a memory leak in c2c_he_free() by calling
perf_hpp__reset_output_field() before freeing the hists container. This
properly unregisters and frees all format entries that were allocated by
hpp_list__parse().

Closes: https://sashiko.dev/finding/41

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index e205f58b2f3d3..07c7e8fb315e6 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -185,6 +185,7 @@ static void c2c_he_free(void *he)
>  	c2c_he = container_of(he, struct c2c_hist_entry, he);
>  	if (c2c_he->hists) {
>  		hists__delete_entries(&c2c_he->hists->hists);

[Severity: High]
This is a pre-existing issue, but does this adequately clear out the inner
entries?

During the tracking phase, node-level events are inserted into the inner
hists of each cacheline c2c_hist_entry, populating
hists->entries_in_array[0]. When perf c2c resorts the main cachelines, it
may merge entries and delete the redundant ones by invoking c2c_he_free().

At this stage, the inner hists of the redundant entries have not yet been
output-resorted, so their entries tree is empty. Because
hists__delete_entries() only cleans the empty entries tree, could the
subsequent zfree(&c2c_he->hists) leak all inner hist_entry objects in
entries_in_array[0] or entries_collapsed?

> +		perf_hpp__reset_output_field(&c2c_he->hists->list);
>  		zfree(&c2c_he->hists);
>  	}

[Severity: Medium]
This is a pre-existing issue, but is there a similar leak of the format
list entries on the error path in he__get_c2c_hists()?

tools/perf/builtin-c2c.c:he__get_c2c_hists() {
    ...
	ret = c2c_hists__init(hists, sort, nr_header_lines, env);
	if (ret) {
		c2c_he->hists = NULL;
		free(hists);
		return NULL;
	}
    ...
}

If c2c_hists__init() fails partway through after appending some format
entries to hists->list, they don't seem to be cleared before freeing the
hists container. Should perf_hpp__reset_output_field() be called here as
well?

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