[PATCHES 0/2] perf c2c hardening

[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 1/2] perf c2c: Fix error masking and OOM handling 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 by checking strdup() return values before proceeding, and preserving
the first error across PARSE_LIST calls by checking ret and jumping to
the cleanup label.

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-opus-4.6
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/builtin-c2c.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
index c9584dbedf77afe8..332d360ca533424b 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;
-- 
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

Re: [PATCH 1/2] perf c2c: Fix error masking and OOM handling in hpp_list__parse()

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The patch introduces early exits in `hpp_list__parse` on parsing errors, skipping the critical `perf_hpp__setup_output_field` initialization. Callers like `c2c_hists__reinit` ignore this error, causing histograms to be rendered without necessary output fields.
- [Low] Missing explicit inclusion of `<string.h>` and `<stdlib.h>` for POSIX libc functions introduced by the patch.
--

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

perf c2c: Fix error masking and OOM handling in hpp_list__parse()

This commit fixes error masking in hpp_list__parse() by checking strdup()
return values and preserving the first error across PARSE_LIST calls.
It prevents silently treating OOM failures as empty input.

> diff --git a/tools/perf/builtin-c2c.c b/tools/perf/builtin-c2c.c
> index c9584dbedf77a..332d360ca5334 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;

[Severity: Low]
Does this file need explicit inclusions for string.h and stdlib.h?

The perf subsystem guidelines require standard POSIX inclusions for musl
compatibility when using functions like strdup() and free().

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

[Severity: Medium]
Does skipping perf_hpp__setup_output_field() leave the output fields list
uninitialized when an error occurs?

Callers of hpp_list__parse(), such as resort_cl_cb() and perf_c2c__report(),
appear to ignore its return value. For example:

tools/perf/builtin-c2c.c:resort_cl_cb() {
    ...
    c2c_hists__reinit(c2c_hists, c2c.cl_output, c2c.cl_resort, env);
    ...
}

tools/perf/builtin-c2c.c:perf_c2c__report() {
    ...
    c2c_hists__reinit(&c2c.hists, output_str, sort_str, 
                      perf_session__env(session));
    ...
}

If an error is returned due to invalid keys or OOM, the callers proceed with
a broken state, which could lead to rendered histograms without necessary 
output fields.

[ ... ]

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

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