Re: [PATCH v4 07/14] perf stat: Implement CSV formatting callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect `realloc` error handling in `csv_metric_only_print_end` leads to a memory leak and segmentation fault.
- [Medium] The buffered cgroup name is silently dropped in `csv_print_end()`, breaking cgroup reporting in CSV mode.
--

--- Patch [7]: [PATCH v4 07/14] perf stat: Implement CSV formatting callbacks ---
commit 20a60f8e4c4914f0bb470217a8770175ba0dfe51
Author: Ian Rogers <irogers@google.com>
Subject: perf stat: Implement CSV formatting callbacks

This commit implements CSV output formatting callbacks in util/stat-print-csv.c.
It defines structures to buffer traversal streams and encapsulates CSV queued
lists lifecycle inside the print start and end functions. It also incorporates
full interval-mode timestamp printing support.

> diff --git a/tools/perf/util/stat-print-csv.c b/tools/perf/util/stat-print-csv.c
> index ee86d07636ea6..f93ca1885d468 100644
> --- a/tools/perf/util/stat-print-csv.c
> +++ b/tools/perf/util/stat-print-csv.c

[ ... ]

> +static int csv_print_end(void *ctx, struct perf_stat_config *config)
> +{
> +	struct csv_print_state *ps = ctx;
> +	struct queued_event *ev, *tmp_ev;
> +	struct queued_metric *met, *tmp_met;
> +	FILE *output = ps->fp;
> +	const char *sep = ps->sep;
> +	bool has_metrics;
> +
> +	list_for_each_entry_safe(ev, tmp_ev, &ps->events_list, list) {
> +		struct evsel *evsel = ev->evsel;
> +		bool ok = (ev->run != 0 && ev->ena != 0);
> +		const char *bad_count = ev->supported ? CNTR_NOT_COUNTED : CNTR_NOT_SUPPORTED;
> +		double enabled_percent = 100;
> +
> +		/* Print interval timestamp first if configured */
> +		if (config->interval && ps->timestamp[0])
> +			fprintf(output, "%s", ps->timestamp);

[ ... ]

> +		/* 2. Print Runtime and Enabled Percentage (Columns 4, 5) */
> +		if (ev->run != ev->ena)
> +			enabled_percent = 100.0 * ev->run / ev->ena;
> +		fprintf(output, "%s%" PRIu64 "%s%.2f", sep, ev->run, sep,
> +			enabled_percent);
> +

[Severity: Medium]
Does this code silently drop the buffered cgroup name from the CSV output?

The cgroup string is correctly duplicated and stored in ev->cgrp during
csv_print_event(), but it doesn't appear to be printed here along with the
other core event properties before the ev struct is freed at the end of the
loop.

> +		/* 3. Print Metrics (Columns 6, 7) */
> +		has_metrics = false;
> +		list_for_each_entry_safe(met, tmp_met, &ev->metrics_list, list) {

[ ... ]

> +static int csv_metric_only_print_end(void *ctx, struct perf_stat_config *config)
> +{
> +	struct csv_metric_only_print_state *ps = ctx;

[ ... ]

> +		if (!found_metric) {
> +			if (num_unique >= unique_alloc) {
> +				unique_alloc = unique_alloc ? unique_alloc * 2 : 16;
> +				unique_names = realloc(unique_names,
> +						       unique_alloc * sizeof(char *));
> +				if (!unique_names) {
> +					ret = -ENOMEM;
> +					goto cleanup_grid;
> +				}
> +			}

[Severity: Medium]
Can this cause a memory leak and a segmentation fault if realloc() fails?

If realloc() fails, it returns NULL and directly overwrites the unique_names
pointer, which loses the reference to the previously allocated memory block.

Later, at the cleanup_grid label:

> +cleanup_grid:
> +	for (i = 0; i < num_unique; i++)
> +		free(unique_names[i]);

The code will dereference the now-NULL unique_names array, which would cause an
immediate crash. 

This same pattern also appears just below this for the unique_aggrs allocation:

> +			if (num_aggrs >= aggr_alloc) {
> +				aggr_alloc = aggr_alloc ? aggr_alloc * 2 : 16;
> +				unique_aggrs = realloc(unique_aggrs,
> +						       aggr_alloc * sizeof(int));
> +				if (!unique_aggrs) {
> +					ret = -ENOMEM;
> +					goto cleanup_grid;
> +				}
> +			}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260716070303.507066-1-irogers@google.com?part=7