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

Pre-existing issues:
- [High] perf_hpp__setup_output_field() unconditionally appends missing sort keys to the global perf_hpp_list instead of the provided local list.
- [Medium] Error return from PARSE_LIST(output, ...) is silently overwritten by PARSE_LIST(sort, ...) in hpp_list__parse().
- [Medium] Out-of-memory failures during string duplication are silently masked as empty inputs in hpp_list__parse() and kvm_hpp_list__parse().
--

commit 82a61a9eafaa1d6dd28dc01f46106f2ec1acc81e
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
ensures that dynamically allocated format structures are correctly cleaned up by
calling perf_hpp__reset_output_field() before freeing the hists container.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 07c7e8fb315e6..eabb922ef295e 100644
> --- 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 is a pre-existing issue, but I noticed something when looking at
c2c_hists__init() -> hpp_list__parse().

When an invalid --fields key is provided, the first PARSE_LIST(output, ...)
macro correctly sets ret to an error code and breaks its internal loop.

However, the subsequent PARSE_LIST(sort, ...) macro invocation immediately
resets ret = 0:

tools/perf/builtin-c2c.c:hpp_list__parse() {
    ...
    PARSE_LIST(output, c2c_hists__init_output);
    PARSE_LIST(sort,   c2c_hists__init_sort);
    ...
}

Does this silently swallow the error from the output parsing, returning
success and allowing execution to proceed with an invalid configuration?

[Severity: Medium]
This isn't a bug introduced by this patch, but while reviewing the parsing
logic in hpp_list__parse() and kvm_hpp_list__parse(), I noticed how strdup()
failures are handled.

If strdup() fails due to memory exhaustion, the resulting variables become NULL:

tools/perf/builtin-c2c.c:hpp_list__parse() {
    char *output = output_ ? strdup(output_) : NULL;
    char *sort   = sort_   ? strdup(sort_) : NULL;
    ...
    PARSE_LIST(output, c2c_hists__init_output);
}

Since PARSE_LIST short-circuits a NULL list to success (0), does this mask
out-of-memory failures as empty inputs, skipping configuration entirely
instead of properly aborting?

[Severity: High]
This is a pre-existing issue, but looking deeper into the initialization
path, perf_hpp__setup_output_field() seems to append missing sort keys
to the wrong list.

The function iterates over the local list->sorts, but then calls
perf_hpp__column_register(fmt):

tools/perf/ui/hist.c:perf_hpp__setup_output_field() {
    perf_hpp_list__for_each_sort_list(list, fmt) {
        ...
        perf_hpp__column_register(fmt);
next:
        continue;
    }
}

This inline wrapper explicitly targets the global perf_hpp_list:

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

Could this pollute the global list with pointers to dynamically allocated
format structures owned by the local c2c_hists or kvm_hists?

If so, does this lead to use-after-free vulnerabilities for concurrent
global list accessors when the local list is destroyed?

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

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