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

On Mon, Aug 03, 2026 at 03:02:29PM +0000, [email protected] wrote:
> 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 <[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 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?
 

Indeed, I'll use str_error_r() in v5.

- Arnaldo
> > +			goto out;
> > +		}
> > +	}
> > +out:
> > +	return ret;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1