Re: [RFC PATCH v1 02/14] perf stat: Implement standard console (STD) formatting callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Skipping a zero-valued counter leaves a stale `current_event` pointer, causing a false temporal coupling violation and aborting the printing process.
- [High] Unchecked `aggr_idx` parameter in `should_skip_zero_counter` can lead to an out-of-bounds array access.
- [Medium] Queued events and metrics are leaked if the traversal callback chain fails early.
- [Medium] In interval mode with `--metric-only`, metric column headers are repeatedly printed for every interval.
--

commit 574c7c40ab2b1b89dd11b7fbe55c2b8a5b292b35
Author: Ian Rogers <irogers@google.com>

perf stat: Implement standard console (STD) formatting callbacks

This patch implements standard console formatting callbacks inside
util/stat-print-std.c. It uses buffered DOM nodes for streams and
centralizes aggregation helpers for prefixes and interval-mode timestamp
support.

> diff --git a/tools/perf/util/stat-print-std.c b/tools/perf/util/stat-print-std.c
> index 83987e97c8895..0aa4e2669bbaa 100644
> --- a/tools/perf/util/stat-print-std.c
> +++ b/tools/perf/util/stat-print-std.c

[ ... ]

> +static bool should_skip_zero_counter(const struct perf_stat_config *config, struct evsel *counter,
> +				     int aggr_idx)
> +{
> +	struct perf_cpu cpu;
> +	unsigned int idx;
> +	struct aggr_cpu_id id;
> +
> +	if (verbose == 0 && counter->skippable && !counter->supported)
> +		return true;
> +
> +	if (config->metric_only)
> +		return false;
> +
> +	if (config->aggr_mode == AGGR_THREAD && config->system_wide)
> +		return true;
> +
> +	if (!config->aggr_map || !config->aggr_get_id)
> +		return false;
> +
> +	id = config->aggr_map->map[aggr_idx];

[Severity: High]
Can this result in an out-of-bounds array access?

When using global aggregation modes or unaggregated events, aggr_idx can
be -1. If so, config->aggr_map->map[aggr_idx] would read before the array
base.

> +
> +	if (evsel__is_tool(counter)) {
> +		struct aggr_cpu_id own_id = config->aggr_get_id((struct perf_stat_config *)config,

[ ... ]

> +static int std_print_event(void *ctx, const struct perf_stat_config *config, struct evsel *evsel,
> +			   int aggr_idx, u64 val, u64 ena, u64 run, double stdev_pct)
> +{
> +	struct std_print_state *ps = ctx;
> +	struct queued_event *ev;
> +
> +	/* Skip zero counters locally in STD callbacks if they qualify */
> +	if (val == 0 && should_skip_zero_counter(config, evsel, aggr_idx))
> +		return 0;

[Severity: High]
Does returning early here leave ps->current_event pointing to the
previous event?

If the traversal driver synchronously invokes std_print_metric() for this
skipped event, it will compare the passed evsel against the old
ps->current_event->evsel, triggering the temporal coupling violation
error and aborting the print.

> +
> +	ev = malloc(sizeof(*ev));
> +	if (!ev)
> +		return -ENOMEM;

[ ... ]

> +static int std_metric_only_print_end(void *ctx, const struct perf_stat_config *config)
> +{
> +	struct std_metric_only_print_state *ps = ctx;
> +	struct queued_metric *b, *tmp;
> +	FILE *out = ps->fp;
> +	int first_aggr = -1;
> +	int current_aggr = -1;
> +	const char *color;
> +	char *str;
> +	int mlen;
> +	int ret = 0;
> +	int err;
> +
> +	if (list_empty(&ps->queued_metrics))
> +		return 0;
> +
> +	/* Print the formatted header prefix */
> +	if (!config->interval)
> +		print_header_std(config, ps->target, ps->argc, ps->argv);
> +
> +	first_aggr = list_first_entry(&ps->queued_metrics, struct queued_metric, list)->aggr_idx;
> +
> +	/* Print headers */
> +	list_for_each_entry(b, &ps->queued_metrics, list) {

[Severity: Medium]
Is it intended for the metric column headers to print unconditionally here?

In interval mode, perf_stat__print_cb() calls this end callback
repeatedly, which appears to cause the header row to print before every
interval's row of values instead of just once at the beginning.

> +		if (b->aggr_idx == first_aggr) {
> +			char *header_name;
> +			if (b->unit && b->unit[0]) {

[ ... ]

> +int perf_stat__print_std(struct evlist *evlist, const struct perf_stat_config *config,
> +			 const struct target *target, const struct timespec *ts, int argc,
> +			 const char **argv)
> +{
> +	struct std_print_state ps = {
> +		.fp = config->output,
> +		.target = target,
> +		.argc = argc,
> +		.argv = argv,
> +	};
> +
> +	if (config->metric_only) {
> +		struct std_metric_only_print_state mops = {
> +			.fp = config->output,
> +			.target = target,
> +			.argc = argc,
> +			.argv = argv,
> +		};
> +		if (config->interval && ts) {
> +			scnprintf(mops.timestamp, sizeof(mops.timestamp), "%6lu.%09lu ",
> +				  (unsigned long)ts->tv_sec, ts->tv_nsec);
> +		} else {
> +			mops.timestamp[0] = '\0';
> +		}
> +		return perf_stat__print_cb(evlist, config, target, ts, argc, argv,
> +					   &std_metric_only_print_callbacks, &mops);

[Severity: Medium]
Does this leak the queued events and metrics if perf_stat__print_cb()
returns an error?

The local state structures contain linked lists buffering dynamically
allocated items. If the callback chain fails early (e.g., from an ENOMEM
or EINVAL), the function returns here without executing a cleanup path.

> +	} else {
> +		if (config->interval && ts) {
> +			scnprintf(ps.timestamp, sizeof(ps.timestamp), "%6lu.%09lu ",
> +				  (unsigned long)ts->tv_sec, ts->tv_nsec);
> +		} else {
> +			ps.timestamp[0] = '\0';
> +		}
> +		return perf_stat__print_cb(evlist, config, target, ts, argc, argv,
> +					   &std_print_callbacks, &ps);

[Severity: Medium]
Similarly, does this leak the queued events in ps.events_list if
perf_stat__print_cb() returns an error?

> +	}
> +}

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