Re: [PATCH 1/2] perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse()

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

New issues:
- [Medium] The newly added error handling `if (ret) goto out;` is bypassed because the `PARSE_LIST` macro silently masks non-terminal errors (like OOM) from individual tokens, undermining the commit's intended fix.

Pre-existing issues:
- [Low] Missing explicit POSIX standard header inclusions for libc functions.
--

--- Patch [1]: [PATCH 1/2] perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse() ---
commit 840153db94480f61d734e3e7494d6e8309bb2f8f
Author: Arnaldo Carvalho de Melo <[email protected]>

perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse()
    
This commit addresses error handling bugs in hpp_list__parse() where output
parsing errors were silently overwritten and OOM conditions for strdup() were
ignored. It also fixes unhandled return values from c2c_hists__reinit() and
hists__iterate_cb() in the calling functions.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..df07e97155df2 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;
>  	int ret;
>  
> +	/* strdup() returns NULL on OOM, don't silently treat as empty */
> +	if ((output_ && !output) || (sort_ && !sort)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}

[Severity: Low]
This is a pre-existing issue, but does this file correctly include the POSIX
standard headers for the libc functions it uses? Functions like strdup() and
free() require <string.h> and <stdlib.h>. Relying on implicit inclusion
can cause build failures on musl libc systems.

>  	PARSE_LIST(output, c2c_hists__init_output);
> +	if (ret)
> +		goto out;

[Severity: Medium]
Will this properly catch all errors from the parsing callback? Looking at
the PARSE_LIST macro definition:

	ret = _fn(hpp_list, tok, env);
	if (ret == -EINVAL) {
		pr_err("Invalid --fields key: '%s'", tok);
		break;
	} else if (ret == -ESRCH) {
		pr_err("Unknown --fields key: '%s'", tok);
		break;
	}

If the callback returns an error like -1 due to an allocation failure on a
token that isn't the last in the list, the loop won't break. Does this mean
subsequent iterations could succeed and overwrite ret to 0, causing the newly
added if (ret) goto out; check to be bypassed?

>  	PARSE_LIST(sort,   c2c_hists__init_sort);
> +	if (ret)
> +		goto out;
>

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