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

Pre-existing issues:
- [High] The `PARSE_LIST` macro and `hpp_list__parse()` silently mask and overwrite parsing and memory allocation errors.
- [High] Silent failure on `strdup()` memory allocation failures in `hpp_list__parse()`.
- [High] `perf c2c` lacks `.feature` and `.build_id` callbacks in `struct perf_tool`, breaking pipe mode.
- [High] `perf_hpp__setup_output_field` unconditionally adds sort fields to the global `perf_hpp_list`.
--

commit 60f91534e79bfc7b6b6e9bfb14eaa60cfbd02be4
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf c2c: Free format list entries when c2c_hists__init() fails

When c2c_hists__init() fails partway through hpp_list__parse(),
dynamically allocated format structures that were already added to
hists->list are leaked because he__get_c2c_hists() frees the hists
container without first unregistering the format entries.

> 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: High]
This isn't a bug introduced by this patch, but looking at how
c2c_hists__init() calls hpp_list__parse(), it seems parsing and memory
allocation failures can be silently masked.

Inside hpp_list__parse(), PARSE_LIST() initializes ret = 0. When it is
invoked sequentially:

    PARSE_LIST(output, c2c_hists__init_output);
    PARSE_LIST(sort,   c2c_hists__init_sort);

If the first macro fails (e.g., returning -EINVAL), the second macro
invocation overwrites the error code. Additionally, the macro's loop only
breaks on -EINVAL or -ESRCH, failing to catch -ENOMEM. Could this result
in hpp_list__parse() returning success even when field initialization
fails?

[Severity: High]
This is a pre-existing issue, but I noticed another potential problem inside
hpp_list__parse() during memory allocation:

    char *output = output_ ? strdup(output_) : NULL;
    char *sort   = sort_   ? strdup(sort_) : NULL;

If strdup() fails due to OOM, it returns NULL. The code doesn't check
for NULL before passing it to PARSE_LIST(), which then evaluates
if (!_list) break; and exits with ret = 0. Does this cause the function
to silently set up empty dimension lists and report success rather than
propagating -ENOMEM?

[Severity: High]
This is also a pre-existing issue, but when c2c_hists__init() sets up
the output fields, there might be a use-after-free risk related to the
global list.

In perf_hpp__setup_output_field(), when adding a new field, it calls:

    perf_hpp__column_register(fmt);

This wrapper unconditionally targets the global perf_hpp_list:

    perf_hpp_list__column_register(&perf_hpp_list, format);

Since hists->list is a dynamically allocated local list, does this end
up polluting the global list with local format pointers that might later
be freed on error paths?

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

[Severity: High]
This is a pre-existing issue, but while looking at perf c2c tool
initialization in cmd_c2c(), I noticed it lacks .feature and .build_id
callbacks.

    perf_tool__init(&c2c.tool, /*ordered_events=*/true);
    c2c.tool.sample = process_sample_event;

In pipe mode, system features like NUMA nodes are synthesized into
PERF_RECORD_HEADER_FEATURE events. Because the feature callback is
missing, perf_env isn't populated.

Later, setup_nodes() checks:

    n = env->numa_nodes;
    if (!n)
        return -EINVAL;

Does the missing feature callback cause piped inputs and cross-analysis
to completely fail by leaving env->numa_nodes as 0?

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