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 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 messa=
ges.
--

--- 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 <[email protected]>

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 e=
rrors
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_l=
ist *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 pe=
rf_env *env))
> +{
> +	char *tmp, *tok;
> +	int ret =3D 0;
> +
> +	if (!_list)
> +		return 0;
> +
> +	for (tok =3D strtok_r(_list, ", ", &tmp); tok; tok =3D strtok_r(NULL, "=
, ", &tmp)) {
> +		ret =3D _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), bu=
t it
does not necessarily set errno.

Even if it did, if the callback or its callees log anything, functions call=
ed
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 ins=
tead
of relying on the implicit global errno variable?

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

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260803144119.1856=
[email protected]?part=3D1