[PATCHES v3 0/2] perf c2c hardening

[PATCHES v3 0/2] perf c2c hardening

[Arnaldo Carvalho de Melo]

Hi,

Fixes for perf c2c found by the sashiko-bot AI reviewer.

Patch 1 fixes three silent failure modes in hpp_list__parse().

Patch 2 fixes a format list leak: when c2c_hists__init() fails partway
through, entries registered via perf_hpp_list__column_register() and
perf_hpp_list__register_sort_field() are left on the hpp_list.

Please consider merging,

- Arnaldo

Changes since v2:

  - Added the string.h and stdlib.h missing headers.
  - Addressed sashiko comment on PARSE_LIST, turning it into a function
    and handling all errors returned from _fn().

Changes since v1:

  - Addressed sashiko-bot [Medium] finding: the early exits added by v1
    skip perf_hpp__setup_output_field().
  - Dismissed sashiko-bot [Low] finding (missing string.h/stdlib.h):
    strdup() and free() were already used in this function before this
    patch; string2.h (included at the top of the file) pulls in string.h.
    No new library calls were introduced.
  - Subject updated to reflect the additional caller fixes.

This series was developed with AI assistance (Claude Sonnet 4.6).

Arnaldo Carvalho de Melo (2):
  perf c2c: Fix error masking, OOM, and unchecked caller errors in
    hpp_list__parse()
  perf c2c: Clean up registered formats on c2c_hists__init() failure

 tools/perf/builtin-c2c.c | 88 ++++++++++++++++++++++++++++------------
 tools/perf/util/sort.c   | 38 +++++++++--------
 2 files changed, 84 insertions(+), 42 deletions(-)

-- 
2.55.0

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

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

hpp_list__parse() has three bugs:

1. The PARSE_LIST macro resets ret = 0 at the start of each invocation,
   so an error from output parsing is silently overwritten when the sort
   parsing block runs.  The function returns success with partially
   initialized state.

2. When the caller passes a non-NULL output_ or sort_ string, but
   strdup() returns NULL due to OOM, NULL is passed to PARSE_LIST which
   treats it as empty input (the "if (!_list) break" branch).  No error
   is returned.

3. When the called _fn function fails and returns something other than
   -ESRCH or -EINVAL (-ENOMEM, for instance) it was not bailing out of
   the strtok loop.

Fix them by checking strdup() return values before proceeding and adding
a cleanup label so that ret from each PARSE_LIST call is checked before
the next runs, preserving the first error.

The early exits now skip perf_hpp__setup_output_field(), which means
c2c_hists__reinit() can return a non-zero value in cases that previously
always succeeded silently.  Both callers discarded its return:
resort_cl_cb() continued into hists__collapse_resort() on a broken list,
and perf_c2c__report() proceeded with uninitialised hists.  Fix the full
chain: check and propagate the error in resort_cl_cb() -- hists__iterate_cb()
already stops iteration and returns the callback error -- and check both
c2c_hists__reinit() and hists__iterate_cb() in perf_c2c__report().

Also turn PARSE_LIST into a function, using a switch to catch other
errors, converting the called functions to return an appropriate errno
instead of -1 on failure.

Fixes: 2d388bd0c9d3 ("perf c2c report: Add stdio output support")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude:claude-sonnet-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 80 +++++++++++++++++++++++++++-------------
 tools/perf/util/sort.c   | 38 +++++++++++--------
 2 files changed, 77 insertions(+), 41 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c9584dbedf77afe8..0f2fb13e705e3712 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -12,6 +12,8 @@
  */
 #include <errno.h>
 #include <inttypes.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include <asm/bug.h>
 #include <linux/compiler.h>
@@ -2063,26 +2065,34 @@ static int c2c_hists__init_sort(struct perf_hpp_list *hpp_list, char *name, stru
 	return 0;
 }
 
-#define PARSE_LIST(_list, _fn)							\
-	do {									\
-		char *tmp, *tok;						\
-		ret = 0;							\
-										\
-		if (!_list)							\
-			break;							\
-										\
-		for (tok = strtok_r((char *)_list, ", ", &tmp);			\
-				tok; tok = strtok_r(NULL, ", ", &tmp)) {	\
-			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;						\
-			}							\
-		}								\
-	} while (0)
+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;
+		}
+	}
+
+	return ret;
+}
 
 static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 			   const char *output_,
@@ -2093,8 +2103,18 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 	char *sort   = sort_   ? strdup(sort_) : NULL;
 	int ret;
 
-	PARSE_LIST(output, c2c_hists__init_output);
-	PARSE_LIST(sort,   c2c_hists__init_sort);
+	/* strdup() returns NULL on OOM, don't silently treat as empty */
+	if ((output_ && !output) || (sort_ && !sort)) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
+	ret = __hpp_list__parse(hpp_list, output, env, c2c_hists__init_output);
+	if (ret)
+		goto out;
+	ret = __hpp_list__parse(hpp_list, sort, env, c2c_hists__init_sort);
+	if (ret)
+		goto out;
 
 	/* copy sort keys to output fields */
 	perf_hpp__setup_output_field(hpp_list);
@@ -2111,6 +2131,7 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 	perf_hpp__append_sort_keys(&hists->list);
 #endif
 
+out:
 	free(output);
 	free(sort);
 	return ret;
@@ -2281,6 +2302,7 @@ static int resort_cl_cb(struct hist_entry *he, void *arg)
 	struct c2c_hist_entry *c2c_he;
 	struct c2c_hists *c2c_hists;
 	bool display = he__display(he, &c2c.shared_clines_stats);
+	int ret;
 
 	c2c_he = container_of(he, struct c2c_hist_entry, he);
 	c2c_hists = c2c_he->hists;
@@ -2291,7 +2313,9 @@ static int resort_cl_cb(struct hist_entry *he, void *arg)
 		c2c_he->cacheline_idx = idx++;
 		calc_width(c2c_he);
 
-		c2c_hists__reinit(c2c_hists, c2c.cl_output, c2c.cl_resort, env);
+		ret = c2c_hists__reinit(c2c_hists, c2c.cl_output, c2c.cl_resort, env);
+		if (ret)
+			return ret;
 
 		hists__collapse_resort(&c2c_hists->hists, NULL);
 		hists__output_resort_cb(&c2c_hists->hists, NULL, filter_cb);
@@ -3356,13 +3380,19 @@ static int perf_c2c__report(int argc, const char **argv)
 	else if (c2c.display == DISPLAY_SNP_PEER)
 		sort_str = "tot_peer";
 
-	c2c_hists__reinit(&c2c.hists, output_str, sort_str, perf_session__env(session));
+	err = c2c_hists__reinit(&c2c.hists, output_str, sort_str, perf_session__env(session));
+	if (err) {
+		pr_err("Failed to reinitialize hists\n");
+		goto out_mem2node;
+	}
 
 	ui_progress__init(&prog, c2c.hists.hists.nr_entries, "Sorting...");
 
 	hists__collapse_resort(&c2c.hists.hists, NULL);
 	hists__output_resort_cb(&c2c.hists.hists, &prog, resort_shared_cl_cb);
-	hists__iterate_cb(&c2c.hists.hists, resort_cl_cb, perf_session__env(session));
+	err = hists__iterate_cb(&c2c.hists.hists, resort_cl_cb, perf_session__env(session));
+	if (err)
+		goto out_mem2node;
 
 	ui_progress__finish();
 
diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
index dcf9189786f8aeba..2f59e37089ef06a2 100644
--- a/tools/perf/util/sort.c
+++ b/tools/perf/util/sort.c
@@ -3105,7 +3105,7 @@ static int __sort_dimension__add_hpp_sort(struct sort_dimension *sd,
 	struct hpp_sort_entry *hse = __sort_dimension__alloc_hpp(sd, level);
 
 	if (hse == NULL)
-		return -1;
+		return -ENOMEM;
 
 	perf_hpp_list__register_sort_field(list, &hse->hpp);
 	return 0;
@@ -3118,7 +3118,7 @@ static int __sort_dimension__add_hpp_output(struct sort_dimension *sd,
 	struct hpp_sort_entry *hse = __sort_dimension__alloc_hpp(sd, level);
 
 	if (hse == NULL)
-		return -1;
+		return -ENOMEM;
 
 	perf_hpp_list__column_register(list, &hse->hpp);
 	return 0;
@@ -3742,14 +3742,18 @@ static int __sort_dimension__add(struct sort_dimension *sd,
 				 struct perf_hpp_list *list,
 				 int level)
 {
+	int ret;
+
 	if (sd->taken)
 		return 0;
 
-	if (__sort_dimension__add_hpp_sort(sd, list, level) < 0)
-		return -1;
+	ret = __sort_dimension__add_hpp_sort(sd, list, level);
+	if (ret < 0)
+		return ret;
 
-	if (__sort_dimension__update(sd, list) < 0)
-		return -1;
+	ret = __sort_dimension__update(sd, list);
+	if (ret < 0)
+		return ret;
 
 	sd->taken = 1;
 
@@ -3767,7 +3771,7 @@ static int __hpp_dimension__add(struct hpp_dimension *hd,
 
 	fmt = __hpp_dimension__alloc_hpp(hd, level);
 	if (!fmt)
-		return -1;
+		return -ENOMEM;
 
 	hd->taken = 1;
 	hd->was_taken = 1;
@@ -3779,14 +3783,18 @@ static int __sort_dimension__add_output(struct perf_hpp_list *list,
 					struct sort_dimension *sd,
 					int level)
 {
+	int ret;
+
 	if (sd->taken)
 		return 0;
 
-	if (__sort_dimension__add_hpp_output(sd, list, level) < 0)
-		return -1;
+	ret = __sort_dimension__add_hpp_output(sd, list, level);
+	if (ret < 0)
+		return ret;
 
-	if (__sort_dimension__update(sd, list) < 0)
-		return -1;
+	ret = __sort_dimension__update(sd, list);
+	if (ret < 0)
+		return ret;
 
 	sd->taken = 1;
 	return 0;
@@ -3803,7 +3811,7 @@ static int __hpp_dimension__add_output(struct perf_hpp_list *list,
 
 	fmt = __hpp_dimension__alloc_hpp(hd, level);
 	if (!fmt)
-		return -1;
+		return -ENOMEM;
 
 	hd->taken = 1;
 	perf_hpp_list__column_register(list, fmt);
@@ -3869,8 +3877,7 @@ int sort_dimension__add(struct perf_hpp_list *list, const char *tok,
 				    strlen(tok)))
 			return -EINVAL;
 
-		__sort_dimension__add(sd, list, level);
-		return 0;
+		return __sort_dimension__add(sd, list, level);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(memory_sort_dimensions); i++) {
@@ -3882,8 +3889,7 @@ int sort_dimension__add(struct perf_hpp_list *list, const char *tok,
 		if (sort__mode != SORT_MODE__MEMORY)
 			return -EINVAL;
 
-		__sort_dimension__add(sd, list, level);
-		return 0;
+		return __sort_dimension__add(sd, list, level);
 	}
 
 	for (i = 0; i < ARRAY_SIZE(hpp_sort_dimensions); i++) {
-- 
2.55.0

[PATCH 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When c2c_hists__init() calls hpp_list__parse() and it fails partway
through, format structures registered via perf_hpp_list__column_register()
and perf_hpp_list__register_sort_field() are left on the hpp_list.  The
c2c_he__alloc_hists() caller handles this with
perf_hpp__reset_output_field(),
but perf_c2c_report() does not, leaking the partially registered entries.

Fix by adding cleanup inside c2c_hists__init() itself, so all callers
are protected.

Fixes: 78b275437873 ("perf c2c report: Add sample processing")
Reported-by: sashiko-bot <sashiko-bot@kernel.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Assisted-by: Claude:claude-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 0f2fb13e705e3712..9f6ae3adb7951287 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2142,6 +2142,8 @@ static int c2c_hists__init(struct c2c_hists *hists,
 			   int nr_header_lines,
 			   struct perf_env *env)
 {
+	int ret;
+
 	__hists__init(&hists->hists, &hists->list);
 
 	/*
@@ -2154,7 +2156,13 @@ static int c2c_hists__init(struct c2c_hists *hists,
 	/* Overload number of header lines.*/
 	hists->list.nr_header_lines = nr_header_lines;
 
-	return hpp_list__parse(&hists->list, /*output=*/NULL, sort, env);
+	ret = hpp_list__parse(&hists->list, /*output=*/NULL, sort, env);
+
+	/* Unregister any formats added before the failure point */
+	if (ret)
+		perf_hpp__reset_output_field(&hists->list);
+
+	return ret;
 }
 
 static int c2c_hists__reinit(struct c2c_hists *c2c_hists,
-- 
2.55.0

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

[sashiko-bot]

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

Re: [PATCH 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] `__hpp_list__parse()` contains a bug where breaking out of a switch statement fails to terminate the enclosing for-loop, leading to order-dependent error masking.
--

--- Patch [2]: [PATCH 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure ---
commit 8cd4f405e8cab7ae300b2dd0a747dc102c29836e
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf c2c: Clean up registered formats on c2c_hists__init() failure

When c2c_hists__init() calls hpp_list__parse() and fails partway through,
format structures registered are left on the hpp_list. This adds cleanup
inside c2c_hists__init() itself, so all callers are protected.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..9f6ae3adb7951 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
>  	return 0;
>  }
>  
> -#define PARSE_LIST(_list, _fn)							\
> -	do {									\
> -		char *tmp, *tok;						\
> -		ret = 0;							\
> -										\
> -		if (!_list)							\
> -			break;							\
> -										\
> -		for (tok = strtok_r((char *)_list, ", ", &tmp);			\
> -				tok; tok = strtok_r(NULL, ", ", &tmp)) {	\
> -			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;						\
> -			}							\
> -		}								\
> -	} while (0)
> +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;

[Severity: Medium]
Will this break statement only exit the switch block, rather than the enclosing
for-loop?

In the old PARSE_LIST macro, breaking out of the loop aborted parsing upon the
first invalid key.

With a switch statement, it looks like this will continue parsing remaining
tokens, overwriting ret and masking the earlier error if a subsequent token
succeeds.

Does this need to return immediately or use a goto to exit the loop?

> +		case -ESRCH:
> +			pr_err("Unknown --fields key: `%s'", tok);
> +			break;
> +		default:
> +			pr_err("%m for --fields key: `%s'", tok);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}

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