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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The `__hpp_list__parse()` function fails to break out of the string tokenization loop upon encountering an error, silently overwriting error codes and returning false success.
--

--- Patch [1]: [PATCH 1/2] perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse() ---
commit 9193c764390834e98068c032e1076f1f5f739d5a
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse()

This commit refactors hpp_list__parse() to correctly propagate errors from
tokenization and OOM situations. It replaces the PARSE_LIST macro with a
helper function to preserve and return the first encountered error.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..0f2fb13e705e3 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
[ ... ]
> @@ -2063,26 +2065,34 @@ static int c2c_hists__init_sort(struct perf_hpp_list *hpp_list, char *name, stru
[ ... ]
> +static int __hpp_list__parse(struct perf_hpp_list *hpp_list, char *_list, struct perf_env *env,
> +			     int (*_fn)(struct perf_hpp_list *hpp_list, char *name, struct perf_env *env))
> +{
> +	char *tmp, *tok;
> +	int ret = 0;
> +
> +	if (!_list)
> +		return 0;
> +
> +	for (tok = strtok_r(_list, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
> +		ret = _fn(hpp_list, tok, env);
> +		switch (ret) {
> +		case 0:
> +			continue;
> +		case -EINVAL:
> +			pr_err("Invalid --fields key: `%s'", tok);
> +			break;
> +		case -ESRCH:
> +			pr_err("Unknown --fields key: `%s'", tok);
> +			break;
> +		default:
> +			pr_err("%m for --fields key: `%s'", tok);
> +			break;
> +		}

[Severity: High]
Does this actually break out of the strtok loop on error? In C, a break
statement inside a switch block only exits the switch, not the enclosing
for loop.

If an invalid field is followed by a valid one (e.g.,
--fields invalid_field,valid_field), won't the loop continue, causing _fn()
to return 0 for the valid field and silently overwrite the error code in ret?

> +	}
> +
> +	return ret;
> +}

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