Re: [PATCH 1/2] perf c2c: Fix error masking and OOM handling in hpp_list__parse()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch introduces early exits in `hpp_list__parse` on parsing errors, skipping the critical `perf_hpp__setup_output_field` initialization. Callers like `c2c_hists__reinit` ignore this error, causing histograms to be rendered without necessary output fields.
- [Low] Missing explicit inclusion of `<string.h>` and `<stdlib.h>` for POSIX libc functions introduced by the patch.
--

--- Patch [1]: [PATCH 1/2] perf c2c: Fix error masking and OOM handling in hpp_list__parse() ---
commit 52205c976b0de26bcbb5609f521d63f4dbe4ebec
Author: Arnaldo Carvalho de Melo <[email protected]>

perf c2c: Fix error masking and OOM handling in hpp_list__parse()

This commit fixes error masking in hpp_list__parse() by checking strdup()
return values and preserving the first error across PARSE_LIST calls.
It prevents silently treating OOM failures as empty input.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..332d360ca5334 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c

[ ... ]

> @@ -2093,8 +2093,18 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
>  	char *sort   = sort_   ? strdup(sort_) : NULL;

[Severity: Low]
Does this file need explicit inclusions for string.h and stdlib.h?

The perf subsystem guidelines require standard POSIX inclusions for musl
compatibility when using functions like strdup() and free().

>  	int ret;
>  
> +	/* strdup() returns NULL on OOM, don't silently treat as empty */
> +	if ((output_ && !output) || (sort_ && !sort)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>  	PARSE_LIST(output, c2c_hists__init_output);
> +	if (ret)
> +		goto out;
>  	PARSE_LIST(sort,   c2c_hists__init_sort);
> +	if (ret)
> +		goto out;
>  
>  	/* copy sort keys to output fields */
>  	perf_hpp__setup_output_field(hpp_list);

[Severity: Medium]
Does skipping perf_hpp__setup_output_field() leave the output fields list
uninitialized when an error occurs?

Callers of hpp_list__parse(), such as resort_cl_cb() and perf_c2c__report(),
appear to ignore its return value. For example:

tools/perf/builtin-c2c.c:resort_cl_cb() {
    ...
    c2c_hists__reinit(c2c_hists, c2c.cl_output, c2c.cl_resort, env);
    ...
}

tools/perf/builtin-c2c.c:perf_c2c__report() {
    ...
    c2c_hists__reinit(&c2c.hists, output_str, sort_str, 
                      perf_session__env(session));
    ...
}

If an error is returned due to invalid keys or OOM, the callers proceed with
a broken state, which could lead to rendered histograms without necessary 
output fields.

[ ... ]

> @@ -2111,6 +2121,7 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
>  	perf_hpp__append_sort_keys(&hists->list);
>  #endif
>  
> +out:
>  	free(output);
>  	free(sort);
>  	return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1