[PATCHES v7 0/2] perf c2c hardening

[PATCHES v7 0/2] perf c2c hardening

[Arnaldo Carvalho de Melo]

Hi,

Fixes for 'perf c2c' found by the sashiko-bot and
Opencode:mimo-v2.5-free AI reviewers, both using Chris Mason's
review-prompts:

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

Patch 2 fixes a format list leak: when c2c_hists__[re]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 v6:

- c2c_hists__reinit() has the same leak: it clears the list then calls
  hpp_list__parse(), which can register formats before failing, and
  neither the function nor its callers clean up on failure.  Add the same
  cleanup-on-failure pattern to close this gap.

Changes since v5:

- The second v5 patch didn't got thru, resending the same contents as v5.

Changes since v4:

- Replace %m with str_error_r(-ret) as suggested by Sashiko.

Changes since v3:

 - Use 'goto out' to break out from both the switch and the for loop in
   __hpp_list__parse() as noticed by sashiko.

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,
Opencode:mimo-v2.5-free).


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() and
    c2c_hists__reinit() failure

 tools/perf/builtin-c2c.c | 105 +++++++++++++++++++++++++++++----------
 tools/perf/util/sort.c   |  38 ++++++++------
 2 files changed, 100 insertions(+), 43 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 | 85 ++++++++++++++++++++++++++++------------
 tools/perf/util/sort.c   | 38 ++++++++++--------
 2 files changed, 82 insertions(+), 41 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c9584dbedf77afe8..160b82694d391c50 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -12,11 +12,14 @@
  */
 #include <errno.h>
 #include <inttypes.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include <asm/bug.h>
 #include <linux/compiler.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
+#include <linux/string.h>
 #include <linux/stringify.h>
 #include <linux/zalloc.h>
 #include <sys/param.h>
@@ -2063,26 +2066,38 @@ 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);
+			goto out;
+		case -ESRCH:
+			pr_err("Unknown --fields key: `%s'", tok);
+			goto out;
+		default: {
+			char buf[STRERR_BUFSIZE];
+
+			pr_err("%s for --fields key: `%s'",
+			       str_error_r(-ret, buf, sizeof(buf)), tok);
+			goto out;
+		}
+		}
+	}
+out:
+	return ret;
+}
 
 static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 			   const char *output_,
@@ -2093,8 +2108,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 +2136,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 +2307,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 +2318,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 +3385,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

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:
- [Medium] Silent error masking and inconsistent propagation for -ENOMEM during sort and output token parsing in setup_sort_list() and setup_output_list().
--

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

perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse()
    
This commit fixes three bugs in hpp_list__parse() by checking strdup()
return values and adding a cleanup label to preserve early errors. It also
converts PARSE_LIST to a function, changing called functions to return
appropriate error codes like -ENOMEM instead of -1 on failure.

> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 005e7d85dc4a5..616a583e3d4d6 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c

[ ... ]

> @@ -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++) {

[Severity: Medium]
Since sort_dimension__add() can now return -ENOMEM, could this introduce a
regression where the callers in setup_sort_list() and setup_output_list()
fail to handle the new return value?

Currently, setup_sort_list() only checks for -EINVAL and -ESRCH:

tools/perf/util/sort.c:setup_sort_list() {
    ...
		ret = sort_dimension__add(list, tok, evlist, env, level);
		if (ret == -EINVAL) {
			...
			break;
		} else if (ret == -ESRCH) {
			...
			break;
		}
    ...
}

If sort_dimension__add() returns -ENOMEM, the loop will not break and ret
will be silently overwritten on the next iteration.

A similar issue appears to exist in setup_output_list() which ignores
-ENOMEM from output_field_add().

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

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

[Arnaldo Carvalho de Melo]

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

When c2c_hists__init() or c2c_hists__reinit() 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.

In c2c_hists__init(), 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.

In c2c_hists__reinit(), neither perf_c2c_report() nor resort_cl_cb()
clean up on failure.

Fix by adding cleanup inside both functions themselves, 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
Assisted-by: Opencode:mimo-v2.5-free
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 160b82694d391c50..1856cccbee27818b 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2147,6 +2147,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);
 
 	/*
@@ -2159,7 +2161,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,
@@ -2167,8 +2175,16 @@ static int c2c_hists__reinit(struct c2c_hists *c2c_hists,
 			     const char *sort,
 			     struct perf_env *env)
 {
+	int ret;
+
 	perf_hpp__reset_output_field(&c2c_hists->list);
-	return hpp_list__parse(&c2c_hists->list, output, sort, env);
+	ret = hpp_list__parse(&c2c_hists->list, output, sort, env);
+
+	/* Unregister any formats added before the failure point */
+	if (ret)
+		perf_hpp__reset_output_field(&c2c_hists->list);
+
+	return ret;
 }
 
 #define DISPLAY_LINE_LIMIT  0.001
-- 
2.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Medium] print_pareto() calls hpp_list__parse() on a local stack variable but never cleans it up, leaking dynamically allocated formats.
- [Medium] kvm_hists__init() and kvm_hists__reinit() suffer from identical partial-registration leaks when format parsing fails.
--

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

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

When c2c_hists__init() or c2c_hists__reinit() calls hpp_list__parse()
and it fails partway through, format structures registered are left on
the hpp_list. This commit adds cleanup inside both functions to protect
all callers from leaking partially registered entries.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index 160b82694d391..1856cccbee278 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c

[ ... ]

> @@ -2167,8 +2175,16 @@ static int c2c_hists__reinit(struct c2c_hists *c2c_hists,
>  			     const char *sort,
>  			     struct perf_env *env)
>  {
> +	int ret;
> +
>  	perf_hpp__reset_output_field(&c2c_hists->list);
> -	return hpp_list__parse(&c2c_hists->list, output, sort, env);
> +	ret = hpp_list__parse(&c2c_hists->list, output, sort, env);
> +
> +	/* Unregister any formats added before the failure point */
> +	if (ret)
> +		perf_hpp__reset_output_field(&c2c_hists->list);
> +
> +	return ret;
>  }

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed that print_pareto()
in the same file calls hpp_list__parse() on a local stack variable:

tools/perf/builtin-c2c.c:print_pareto() {
    ...
    ret = hpp_list__parse(&hpp_list, cl_output, /*evlist=*/NULL, env);
    ...
}

Because print_pareto() returns without ever calling 
perf_hpp__reset_output_field(), does it leak all allocated c2c_fmt structures 
on both the success and failure paths?

[Severity: Medium]
This is a pre-existing issue, but kvm_hists__init() and kvm_hists__reinit() in
tools/perf/builtin-kvm.c seem to suffer from identical partial-registration
leaks when format parsing fails:

tools/perf/builtin-kvm.c:kvm_hists__init() {
    ...
    return kvm_hpp_list__parse(&kvm_hists.list, NULL, "ev_name");
}

tools/perf/builtin-kvm.c:kvm_hists__reinit() {
    ...
    return kvm_hpp_list__parse(&kvm_hists.list, output, sort);
}

If parsing fails partway through these functions, will partially allocated
formats remain in the global list without cleanup, replicating the exact leak
pattern fixed here in c2c?

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

[PATCHES v8 0/2] perf c2c hardening

[Arnaldo Carvalho de Melo]

Hi,

	Please consider merging,

Two hardening fixes for perf c2c found by the sashiko-bot AI reviewer.

Patch 1 fixes error masking, OOM, and unchecked caller errors in
hpp_list__parse(): the strdup() return values are now checked, PARSE_LIST
was turned into a function that bails out on any error returned by the
parsing callbacks (including -ENOMEM), and the two callers bailing out
with the newly propagated errors, setup_sort_list() and
setup_output_list(), now break out of their token loops and propagate the
error instead of silently overwriting it on the next iteration.

Patch 2 fixes a format list leak: when c2c_hists__[re]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.  The
fix moves cleanup into c2c_hists__init()/c2c_hists__reinit() themselves
so all callers are protected.

This series was developed with AI assistance (Claude:Sonnet 4.6 and
Opencode:mimo-v2.5-free for the changes up to v7; the changes since v7,
including the setup_sort_list()/setup_output_list() -ENOMEM propagation
fix and the c2c_hists__reinit() cleanup amendment, with
Opencode:DeepSeek-V4-Flash-free).

- Arnaldo

Changes since v7 (20260804185830.228763-1-acme@kernel.org):

  Patch 1:
  - Addressed sashiko-bot [Medium] finding: with sort_dimension__add()
    and output_field_add() now propagating -ENOMEM (and other errors),
    the callers setup_sort_list() and setup_output_list() only checked
    for -EINVAL and -ESRCH, so an allocation failure was silently
    overwritten when the next loop iteration ran.  Both callers now break
    out of their token loops on any error and propagate it.

Changes since v6 (20260803180703.194916-1-acme@kernel.org):

  Patch 2:
  - c2c_hists__reinit() has the same leak: it clears the list then calls
    hpp_list__parse(), which can register formats before failing, and
    neither the function nor its callers clean up on failure.  Add the
    same cleanup-on-failure pattern to close this gap.

Changes since v5 (20260803180118.194519-1-acme@kernel.org):

  - The second v5 patch didn't go through, resending the same contents
    as v5.

Changes since v4 (20260803144119.185637-1-acme@kernel.org):

  - Replace %m with str_error_r(-ret) as suggested by sashiko.

Changes since v3 (20260803120452.181273-1-acme@kernel.org):

  - Use 'goto out' to break out from both the switch and the for loop in
    __hpp_list__parse() as noticed by sashiko.

Changes since v2 (20260803011140.179943-1-acme@kernel.org):

  - 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 (20260802142313.154514-1-acme@kernel.org):

  - Addressed sashiko-bot [Medium] finding: the early exits added by v1
    skip perf_hpp__setup_output_field(), making undetected errors in the
    callers consequential.  Fixed by propagating the error through
    resort_cl_cb() and perf_c2c__report() (hists__iterate_cb() already
    propagated callback return values).
  - 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.

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() and c2c_hists__reinit() failure

 tools/perf/builtin-c2c.c | 105 ++++++++++++++++++++++++++++++----------
 tools/perf/util/sort.c   |  76 ++++++++++++++++++++++-------
 2 files changed, 130 insertions(+), 52 deletions(-)

---


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() and
    c2c_hists__reinit() failure

 tools/perf/builtin-c2c.c | 106 ++++++++++++++++++++++++++++-----------
 tools/perf/util/sort.c   |  76 +++++++++++++++++++---------
 2 files changed, 130 insertions(+), 52 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.

Also make the two callers that iterate sort_dimension__add() and
output_field_add() handle the newly propagated errors: setup_sort_list()
and setup_output_list() only checked for -EINVAL and -ESRCH, so an
-ENOMEM from a failed allocation was silently overwritten by the next
loop iteration.  Break out of the loop and propagate any other error.

The hpp_list__parse() fixes were developed with AI assistance from
Claude:claude-sonnet-4.6, and the setup_sort_list()/setup_output_list()
caller fixes with AI assistance from Opencode:mimo-v2.5-free and
Opencode:DeepSeek-V4-Flash-free.

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
Assisted-by: Opencode:mimo-v2.5-free
Assisted-by: Opencode:DeepSeek-V4-Flash-free
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 85 ++++++++++++++++++++++++++++------------
 tools/perf/util/sort.c   | 76 +++++++++++++++++++++++------------
 2 files changed, 112 insertions(+), 49 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c9584dbedf77afe8..160b82694d391c50 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -12,11 +12,14 @@
  */
 #include <errno.h>
 #include <inttypes.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include <asm/bug.h>
 #include <linux/compiler.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
+#include <linux/string.h>
 #include <linux/stringify.h>
 #include <linux/zalloc.h>
 #include <sys/param.h>
@@ -2063,26 +2066,38 @@ 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);
+			goto out;
+		case -ESRCH:
+			pr_err("Unknown --fields key: `%s'", tok);
+			goto out;
+		default: {
+			char buf[STRERR_BUFSIZE];
+
+			pr_err("%s for --fields key: `%s'",
+			       str_error_r(-ret, buf, sizeof(buf)), tok);
+			goto out;
+		}
+		}
+	}
+out:
+	return ret;
+}
 
 static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 			   const char *output_,
@@ -2093,8 +2108,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 +2136,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 +2307,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 +2318,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 +3385,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..58638ec9ae0ede7f 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++) {
@@ -3973,15 +3979,25 @@ static int setup_sort_list(struct perf_hpp_list *list, char *str,
 			}
 
 			ret = sort_dimension__add(list, tok, evlist, env, level);
-			if (ret == -EINVAL) {
+			switch (ret) {
+			case 0:
+				break;
+			case -EINVAL:
 				if (!cacheline_size() && !strncasecmp(tok, "dcacheline", strlen(tok)))
 					ui__error("The \"dcacheline\" --sort key needs to know the cacheline size and it couldn't be determined on this system");
 				else
 					ui__error("Invalid --sort key: `%s'", tok);
-				break;
-			} else if (ret == -ESRCH) {
+				goto out;
+			case -ESRCH:
 				ui__error("Unknown --sort key: `%s'", tok);
-				break;
+				goto out;
+			default: {
+				char buf[STRERR_BUFSIZE];
+
+				ui__error("%s for --sort key: `%s'",
+					  str_error_r(-ret, buf, sizeof(buf)), tok);
+				goto out;
+			}
 			}
 			prev_level = level;
 		}
@@ -3989,6 +4005,7 @@ static int setup_sort_list(struct perf_hpp_list *list, char *str,
 		level = next_level;
 	} while (tmp);
 
+out:
 	return ret;
 }
 
@@ -4315,15 +4332,26 @@ static int setup_output_list(struct perf_hpp_list *list, char *str)
 	for (tok = strtok_r(str, ", ", &tmp);
 			tok; tok = strtok_r(NULL, ", ", &tmp)) {
 		ret = output_field_add(list, tok, &level);
-		if (ret == -EINVAL) {
-			ui__error("Invalid --fields key: `%s'", tok);
+		switch (ret) {
+		case 0:
 			break;
-		} else if (ret == -ESRCH) {
+		case -EINVAL:
+			ui__error("Invalid --fields key: `%s'", tok);
+			goto out;
+		case -ESRCH:
 			ui__error("Unknown --fields key: `%s'", tok);
-			break;
+			goto out;
+		default: {
+			char buf[STRERR_BUFSIZE];
+
+			ui__error("%s for --fields key: `%s'",
+				  str_error_r(-ret, buf, sizeof(buf)), tok);
+			goto out;
+		}
 		}
 	}
 
+out:
 	return ret;
 }
 
-- 
2.55.0

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

[Ian Rogers]

On Wed, Aug 5, 2026 at 8:11 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote:
>
> 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.
>
> Also make the two callers that iterate sort_dimension__add() and
> output_field_add() handle the newly propagated errors: setup_sort_list()
> and setup_output_list() only checked for -EINVAL and -ESRCH, so an
> -ENOMEM from a failed allocation was silently overwritten by the next
> loop iteration.  Break out of the loop and propagate any other error.
>
> The hpp_list__parse() fixes were developed with AI assistance from
> Claude:claude-sonnet-4.6, and the setup_sort_list()/setup_output_list()
> caller fixes with AI assistance from Opencode:mimo-v2.5-free and
> Opencode:DeepSeek-V4-Flash-free.
>
> 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
> Assisted-by: Opencode:mimo-v2.5-free
> Assisted-by: Opencode:DeepSeek-V4-Flash-free
> Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

> ---
>  tools/perf/builtin-c2c.c | 85 ++++++++++++++++++++++++++++------------
>  tools/perf/util/sort.c   | 76 +++++++++++++++++++++++------------
>  2 files changed, 112 insertions(+), 49 deletions(-)
>
> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77afe8..160b82694d391c50 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -12,11 +12,14 @@
>   */
>  #include <errno.h>
>  #include <inttypes.h>
> +#include <stdlib.h>
> +#include <string.h>
>
>  #include <asm/bug.h>
>  #include <linux/compiler.h>
>  #include <linux/err.h>
>  #include <linux/kernel.h>
> +#include <linux/string.h>
>  #include <linux/stringify.h>
>  #include <linux/zalloc.h>
>  #include <sys/param.h>
> @@ -2063,26 +2066,38 @@ 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);
> +                       goto out;
> +               case -ESRCH:
> +                       pr_err("Unknown --fields key: `%s'", tok);
> +                       goto out;
> +               default: {
> +                       char buf[STRERR_BUFSIZE];
> +
> +                       pr_err("%s for --fields key: `%s'",
> +                              str_error_r(-ret, buf, sizeof(buf)), tok);
> +                       goto out;
> +               }
> +               }
> +       }
> +out:
> +       return ret;
> +}
>
>  static int hpp_list__parse(struct perf_hpp_list *hpp_list,
>                            const char *output_,
> @@ -2093,8 +2108,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 +2136,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 +2307,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 +2318,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 +3385,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..58638ec9ae0ede7f 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++) {
> @@ -3973,15 +3979,25 @@ static int setup_sort_list(struct perf_hpp_list *list, char *str,
>                         }
>
>                         ret = sort_dimension__add(list, tok, evlist, env, level);
> -                       if (ret == -EINVAL) {
> +                       switch (ret) {
> +                       case 0:
> +                               break;
> +                       case -EINVAL:
>                                 if (!cacheline_size() && !strncasecmp(tok, "dcacheline", strlen(tok)))
>                                         ui__error("The \"dcacheline\" --sort key needs to know the cacheline size and it couldn't be determined on this system");
>                                 else
>                                         ui__error("Invalid --sort key: `%s'", tok);
> -                               break;
> -                       } else if (ret == -ESRCH) {
> +                               goto out;
> +                       case -ESRCH:
>                                 ui__error("Unknown --sort key: `%s'", tok);
> -                               break;
> +                               goto out;
> +                       default: {
> +                               char buf[STRERR_BUFSIZE];
> +
> +                               ui__error("%s for --sort key: `%s'",
> +                                         str_error_r(-ret, buf, sizeof(buf)), tok);
> +                               goto out;
> +                       }
>                         }
>                         prev_level = level;
>                 }
> @@ -3989,6 +4005,7 @@ static int setup_sort_list(struct perf_hpp_list *list, char *str,
>                 level = next_level;
>         } while (tmp);
>
> +out:
>         return ret;
>  }
>
> @@ -4315,15 +4332,26 @@ static int setup_output_list(struct perf_hpp_list *list, char *str)
>         for (tok = strtok_r(str, ", ", &tmp);
>                         tok; tok = strtok_r(NULL, ", ", &tmp)) {
>                 ret = output_field_add(list, tok, &level);
> -               if (ret == -EINVAL) {
> -                       ui__error("Invalid --fields key: `%s'", tok);
> +               switch (ret) {
> +               case 0:
>                         break;
> -               } else if (ret == -ESRCH) {
> +               case -EINVAL:
> +                       ui__error("Invalid --fields key: `%s'", tok);
> +                       goto out;
> +               case -ESRCH:
>                         ui__error("Unknown --fields key: `%s'", tok);
> -                       break;
> +                       goto out;
> +               default: {
> +                       char buf[STRERR_BUFSIZE];
> +
> +                       ui__error("%s for --fields key: `%s'",
> +                                 str_error_r(-ret, buf, sizeof(buf)), tok);
> +                       goto out;
> +               }
>                 }
>         }
>
> +out:
>         return ret;
>  }
>
> --
> 2.55.0
>

[PATCHES v6 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 v5:

- The second v5 patch didn't got thru, resending the same contents as v5.

Changes since v4:

- Replace %m with str_error_r(-ret) as suggested by Sashiko.

Changes since v3:

 - Use 'goto out' to break out from both the switch and the for loop in
   __hpp_list__parse() as noticed by sashiko.

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 | 95 +++++++++++++++++++++++++++++-----------
 tools/perf/util/sort.c   | 38 +++++++++-------
 2 files changed, 91 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 | 85 ++++++++++++++++++++++++++++------------
 tools/perf/util/sort.c   | 38 ++++++++++--------
 2 files changed, 82 insertions(+), 41 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c9584dbedf77afe8..160b82694d391c50 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -12,11 +12,14 @@
  */
 #include <errno.h>
 #include <inttypes.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include <asm/bug.h>
 #include <linux/compiler.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
+#include <linux/string.h>
 #include <linux/stringify.h>
 #include <linux/zalloc.h>
 #include <sys/param.h>
@@ -2063,26 +2066,38 @@ 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);
+			goto out;
+		case -ESRCH:
+			pr_err("Unknown --fields key: `%s'", tok);
+			goto out;
+		default: {
+			char buf[STRERR_BUFSIZE];
+
+			pr_err("%s for --fields key: `%s'",
+			       str_error_r(-ret, buf, sizeof(buf)), tok);
+			goto out;
+		}
+		}
+	}
+out:
+	return ret;
+}
 
 static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 			   const char *output_,
@@ -2093,8 +2108,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 +2136,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 +2307,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 +2318,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 +3385,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

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:

Pre-existing issues:
- [Medium] The string parsing loops in `setup_sort_list()` and `setup_output_list()` silently ignore and overwrite memory allocation errors (-ENOMEM), leading to partially initialized sort or output fields being treated as successful.
--

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

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

This commit fixes multiple issues in hpp_list__parse(), including error
masking during string parsing, unhandled strdup() OOM failures, and unchecked
function returns. It also updates underlying sort and output functions to
return -ENOMEM instead of -1 on allocation failures.

> diff --git a/tools/perf/util/sort.c b/tools/perf/util/sort.c
> index 005e7d85dc4a5..616a583e3d4d6 100644
> --- a/tools/perf/util/sort.c
> +++ b/tools/perf/util/sort.c
[ ... ]
> @@ -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++) {

[Severity: Medium]
This is a pre-existing issue, but by explicitly propagating -ENOMEM out of
sort_dimension__add() and output_field_add(), does this highlight a bug in
their callers?

In setup_sort_list() and setup_output_list(), the parsing loops only check
for -EINVAL and -ESRCH:

ret = sort_dimension__add(list, tok, evlist, env, level);
if (ret == -EINVAL) {
	...
	break;
} else if (ret == -ESRCH) {
	...
	break;
}

If sort_dimension__add() or output_field_add() returns -ENOMEM, the loop
continues without breaking, and a subsequent successful token will overwrite
the error. This can cause the perf tool to silently continue with a partially
configured state instead of aborting on memory allocation failure.

Should these loops be updated to catch other error codes like -ENOMEM?

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

[PATCHES v5 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 v4:

- Replace %m with str_error_r(-ret) as suggested by Sashiko.

Changes since v3:

 - Use 'goto out' to break out from both the switch and the for loop in
   __hpp_list__parse() as noticed by sashiko.

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 | 95 +++++++++++++++++++++++++++++-----------
 tools/perf/util/sort.c   | 38 +++++++++-------
 2 files changed, 91 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 | 85 ++++++++++++++++++++++++++++------------
 tools/perf/util/sort.c   | 38 ++++++++++--------
 2 files changed, 82 insertions(+), 41 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c9584dbedf77afe8..160b82694d391c50 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -12,11 +12,14 @@
  */
 #include <errno.h>
 #include <inttypes.h>
+#include <stdlib.h>
+#include <string.h>
 
 #include <asm/bug.h>
 #include <linux/compiler.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
+#include <linux/string.h>
 #include <linux/stringify.h>
 #include <linux/zalloc.h>
 #include <sys/param.h>
@@ -2063,26 +2066,38 @@ 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);
+			goto out;
+		case -ESRCH:
+			pr_err("Unknown --fields key: `%s'", tok);
+			goto out;
+		default: {
+			char buf[STRERR_BUFSIZE];
+
+			pr_err("%s for --fields key: `%s'",
+			       str_error_r(-ret, buf, sizeof(buf)), tok);
+			goto out;
+		}
+		}
+	}
+out:
+	return ret;
+}
 
 static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 			   const char *output_,
@@ -2093,8 +2108,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 +2136,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 +2307,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 +2318,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 +3385,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

[PATCHES v4 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 v3:

 - Use 'goto out' to break out from both the switch and the for loop in
   __hpp_list__parse() as noticed by sashiko.

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 | 90 ++++++++++++++++++++++++++++------------
 tools/perf/util/sort.c   | 38 ++++++++++-------
 2 files changed, 86 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..b0f2ba8318304914 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);
+			goto out;
+		case -ESRCH:
+			pr_err("Unknown --fields key: `%s'", tok);
+			goto out;
+		default:
+			pr_err("%m for --fields key: `%s'", tok);
+			goto out;
+		}
+	}
+out:
+	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

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:
- [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

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

[Arnaldo Carvalho de Melo]

On Mon, Aug 03, 2026 at 03:02:29PM +0000, sashiko-bot@kernel.org 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 <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?
 

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

- Arnaldo
> > +			goto out;
> > +		}
> > +	}
> > +out:
> > +	return ret;
> > +}
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260803144119.185637-1-acme@kernel.org?part=1

[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

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

[PATCHES v2 0/2] perf c2c hardening

[Arnaldo Carvalho de Melo]

Hi,

        Please consider merging,

Two hardening fixes for perf c2c found by the sashiko-bot AI reviewer.

Patch 1 fixes two silent failure modes in hpp_list__parse(): error masking
across successive PARSE_LIST invocations, and OOM from strdup() being
silently treated as empty input.  The fix adds early exits, which exposed
a pre-existing problem: both callers of c2c_hists__reinit() had always
discarded its return value, so errors would now go undetected.  Patch 1
also fixes the full caller chain: resort_cl_cb() now checks and propagates
the error, and perf_c2c__report() checks both c2c_hists__reinit() and
hists__iterate_cb() -- which already stops iteration on callback errors.

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.
perf_c2c_report() does not call perf_hpp__reset_output_field() on the
error path, so those entries leak.  The fix moves cleanup into
c2c_hists__init() itself so all callers are protected.

Changes since v1 (20260802142313.154514-1-acme@kernel.org):

  Patch 1:
  - Addressed sashiko-bot [Medium] finding: the early exits added by v1
    skip perf_hpp__setup_output_field(), making undetected errors in the
    callers consequential.  Fixed by propagating the error through
    resort_cl_cb() and perf_c2c__report() (hists__iterate_cb() already
    propagated callback return values).
  - 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


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 | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 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 two 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.

Fix both 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().

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 | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c9584dbedf77afe8..df07e97155df2584 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2093,8 +2093,18 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
 	char *sort   = sort_   ? strdup(sort_) : NULL;
 	int ret;
 
+	/* strdup() returns NULL on OOM, don't silently treat as empty */
+	if ((output_ && !output) || (sort_ && !sort)) {
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	PARSE_LIST(output, c2c_hists__init_output);
+	if (ret)
+		goto out;
 	PARSE_LIST(sort,   c2c_hists__init_sort);
+	if (ret)
+		goto out;
 
 	/* copy sort keys to output fields */
 	perf_hpp__setup_output_field(hpp_list);
@@ -2111,6 +2121,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 +2292,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 +2303,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 +3370,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();
 
-- 
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 2 potential issue(s) to consider:

New issues:
- [Medium] The newly added error handling `if (ret) goto out;` is bypassed because the `PARSE_LIST` macro silently masks non-terminal errors (like OOM) from individual tokens, undermining the commit's intended fix.

Pre-existing issues:
- [Low] Missing explicit POSIX standard header inclusions for libc functions.
--

--- Patch [1]: [PATCH 1/2] perf c2c: Fix error masking, OOM, and unchecked caller errors in hpp_list__parse() ---
commit 840153db94480f61d734e3e7494d6e8309bb2f8f
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 error handling bugs in hpp_list__parse() where output
parsing errors were silently overwritten and OOM conditions for strdup() were
ignored. It also fixes unhandled return values from c2c_hists__reinit() and
hists__iterate_cb() in the calling functions.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..df07e97155df2 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c
> @@ -2093,8 +2093,18 @@ static int hpp_list__parse(struct perf_hpp_list *hpp_list,
>  	char *sort   = sort_   ? strdup(sort_) : NULL;
>  	int ret;
>  
> +	/* strdup() returns NULL on OOM, don't silently treat as empty */
> +	if ((output_ && !output) || (sort_ && !sort)) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}

[Severity: Low]
This is a pre-existing issue, but does this file correctly include the POSIX
standard headers for the libc functions it uses? Functions like strdup() and
free() require <string.h> and <stdlib.h>. Relying on implicit inclusion
can cause build failures on musl libc systems.

>  	PARSE_LIST(output, c2c_hists__init_output);
> +	if (ret)
> +		goto out;

[Severity: Medium]
Will this properly catch all errors from the parsing callback? Looking at
the PARSE_LIST macro definition:

	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;
	}

If the callback returns an error like -1 due to an allocation failure on a
token that isn't the last in the list, the loop won't break. Does this mean
subsequent iterations could succeed and overwrite ret to 0, causing the newly
added if (ret) goto out; check to be bypassed?

>  	PARSE_LIST(sort,   c2c_hists__init_sort);
> +	if (ret)
> +		goto out;
>

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