[PATCHES v2 0/2] perf c2c hardening

[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

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

[Arnaldo Carvalho de Melo]

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

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

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

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

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index df07e97155df2584..9b59197f01aa642c 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2132,6 +2132,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);
 
 	/*
@@ -2144,7 +2146,13 @@ static int c2c_hists__init(struct c2c_hists *hists,
 	/* Overload number of header lines.*/
 	hists->list.nr_header_lines = nr_header_lines;
 
-	return hpp_list__parse(&hists->list, /*output=*/NULL, sort, env);
+	ret = hpp_list__parse(&hists->list, /*output=*/NULL, sort, env);
+
+	/* Unregister any formats added before the failure point */
+	if (ret)
+		perf_hpp__reset_output_field(&hists->list);
+
+	return ret;
 }
 
 static int c2c_hists__reinit(struct c2c_hists *c2c_hists,
-- 
2.55.0

[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 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure

[Arnaldo Carvalho de Melo]

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

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

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

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

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 160b82694d391c50..a01fa1655569cac9 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,
-- 
2.55.0

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

[sashiko-bot]

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

Pre-existing issues:
- [High] perf_hpp__setup_output_field() and perf_hpp__append_sort_keys() corrupt the global perf_hpp_list and fail to populate the provided local list.
--

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

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

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

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

[ ... ]

> @@ -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);

[Severity: High]
This isn't a bug introduced by this patch, but will calling hpp_list__parse()
corrupt the global perf_hpp_list?

When c2c initializes its local hpp_list, hpp_list__parse() eventually leads
to calls to perf_hpp__setup_output_field() and perf_hpp__append_sort_keys()
in tools/perf/ui/hist.c. Both of these functions accept a local
struct perf_hpp_list *list parameter, but they appear to use the global list
wrappers instead of modifying the passed local list:

tools/perf/ui/hist.c:perf_hpp__setup_output_field() {
    ...
    perf_hpp__column_register(fmt);
    ...
}

tools/perf/ui/hist.c:perf_hpp__append_sort_keys() {
    ...
    perf_hpp__register_sort_field(fmt);
    ...
}

Could this cause the local lists to miss output columns, while also polluting
the global list with dangling pointers to dynamically allocated local formats?

Should these functions be updated to call
perf_hpp_list__column_register(list, fmt) and
perf_hpp_list__register_sort_field(list, fmt) instead?

> +
> +	/* Unregister any formats added before the failure point */
> +	if (ret)
> +		perf_hpp__reset_output_field(&hists->list);
> +
> +	return ret;
>  }
>

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

[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 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure

[Arnaldo Carvalho de Melo]

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

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

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

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

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

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Unconditional memory leak of dynamically allocated format structures in `print_pareto()`.
- [High] Architectural/API Misuse: `perf_hpp__setup_output_field()` and `perf_hpp__append_sort_keys()` silently corrupt the global `perf_hpp_list`.
--

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

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

This commit addresses a memory leak when c2c_hists__init() fails partway
through parsing. It ensures partially registered format structures are
unregistered before returning the error. This protects callers from leaking
memory during report generation.

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

[Severity: High]
This is a pre-existing issue, but does print_pareto() unconditionally leak
dynamically allocated format structures?

Every time print_pareto() is called during C2C reporting, it initializes
a local stack variable for the formats:

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

However, when the function exits on both success and error paths, it doesn't
call perf_hpp__reset_output_field() on hpp_list. Does this mean the formats
attached to this local list are permanently leaked when the list goes out of
scope?

[Severity: High]
This is a pre-existing issue, but do perf_hpp__setup_output_field() and
perf_hpp__append_sort_keys() unintentionally corrupt the global
perf_hpp_list instead of updating the passed local list?

When these functions are called with a local list, they iterate over the
local list but invoke the register functions directly:

tools/perf/ui/hist.c:perf_hpp__setup_output_field() {
        ...
        perf_hpp__column_register(fmt);
        ...
}

tools/perf/ui/hist.c:perf_hpp__append_sort_keys() {
        ...
        perf_hpp__register_sort_field(fmt);
        ...
}

These wrappers appear to be hardcoded to insert elements into the global list:

tools/perf/util/hist.h:
#define perf_hpp__column_register(format) \
        perf_hpp_list__column_register(&perf_hpp_list, format)

Does this leave the required fields missing from the local list, while
simultaneously polluting the global perf_hpp_list state with dynamically
allocated objects?

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

[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 2/2] perf c2c: Clean up registered formats on c2c_hists__init() failure

[Arnaldo Carvalho de Melo]

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

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

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

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

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

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

[sashiko-bot]

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

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

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

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

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..9f6ae3adb7951 100644
> --- a/tools/perf/builtin-c2c.c
> +++ b/tools/perf/builtin-c2c.c

[ ... ]

> @@ -2063,26 +2065,34 @@ static int c2c_hists__init_sort(struct perf_hpp_list *hpp_list, char *name, stru
>  	return 0;
>  }
>  
> -#define PARSE_LIST(_list, _fn)							\
> -	do {									\
> -		char *tmp, *tok;						\
> -		ret = 0;							\
> -										\
> -		if (!_list)							\
> -			break;							\
> -										\
> -		for (tok = strtok_r((char *)_list, ", ", &tmp);			\
> -				tok; tok = strtok_r(NULL, ", ", &tmp)) {	\
> -			ret = _fn(hpp_list, tok, env);				\
> -			if (ret == -EINVAL) {					\
> -				pr_err("Invalid --fields key: `%s'", tok);	\
> -				break;						\
> -			} else if (ret == -ESRCH) {				\
> -				pr_err("Unknown --fields key: `%s'", tok);	\
> -				break;						\
> -			}							\
> -		}								\
> -	} while (0)
> +static int __hpp_list__parse(struct perf_hpp_list *hpp_list, char *_list, struct perf_env *env,
> +			     int (*_fn)(struct perf_hpp_list *hpp_list, char *name, struct perf_env *env))
> +{
> +	char *tmp, *tok;
> +	int ret = 0;
> +
> +	if (!_list)
> +		return 0;
> +
> +	for (tok = strtok_r(_list, ", ", &tmp); tok; tok = strtok_r(NULL, ", ", &tmp)) {
> +		ret = _fn(hpp_list, tok, env);
> +		switch (ret) {
> +		case 0:
> +			continue;
> +		case -EINVAL:
> +			pr_err("Invalid --fields key: `%s'", tok);
> +			break;

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

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

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

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

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

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

[PATCHES 0/2] perf c2c hardening

[Arnaldo Carvalho de Melo]

Hi,

	Please consider merging,

- Arnaldo

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

 tools/perf/builtin-c2c.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

-- 
2.55.0

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

[Arnaldo Carvalho de Melo]

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

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

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

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

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index 332d360ca533424b..84f0f0ba56131b7b 100644
--- a/tools/perf/builtin-c2c.c
+++ b/tools/perf/builtin-c2c.c
@@ -2132,6 +2132,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);
 
 	/*
@@ -2144,7 +2146,13 @@ static int c2c_hists__init(struct c2c_hists *hists,
 	/* Overload number of header lines.*/
 	hists->list.nr_header_lines = nr_header_lines;
 
-	return hpp_list__parse(&hists->list, /*output=*/NULL, sort, env);
+	ret = hpp_list__parse(&hists->list, /*output=*/NULL, sort, env);
+
+	/* Unregister any formats added before the failure point */
+	if (ret)
+		perf_hpp__reset_output_field(&hists->list);
+
+	return ret;
 }
 
 static int c2c_hists__reinit(struct c2c_hists *c2c_hists,
-- 
2.55.0