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:
- [Medium] The `%m` format specifier is used in `__hpp_list__parse` to log generic callback errors, which can result in incorrect or stale error messages.
--

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

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

This commit addresses multiple bugs in hpp_list__parse(). It ensures that errors
from output and sort parsing are not silently masked, handles OOM conditions
from strdup() correctly, and properly propagates failures out of the strtok loop
and up the call chain.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..b0f2ba8318304 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);
> +			goto out;
> +		case -ESRCH:
> +			pr_err("Unknown --fields key: `%s'", tok);
> +			goto out;
> +		default:
> +			pr_err("%m for --fields key: `%s'", tok);

[Severity: Medium]
Will %m print the correct error message here?

The _fn callback returns a negative error code in ret (such as -ENOMEM), but it
does not necessarily set errno.

Even if it did, if the callback or its callees log anything, functions called
internally by pr_err() (like fprintf or localtime_r) can clobber errno even on
success. This could cause %m to print a stale or incorrect error string (e.g.,
"No such file or directory" instead of "Cannot allocate memory").

Could we format the string using the actual error code available in ret instead
of relying on the implicit global errno variable?

> +			goto out;
> +		}
> +	}
> +out:
> +	return ret;
> +}

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