Re: [PATCH v3 10/14] perf stat: Implement streaming JSON formatting callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Metric-only mode ignores the provided `evsel` and instead extracts aggregation topology from an unrelated first event in the evlist.
- [High] Unchecked array access into `ps_evsel->aggr` using a potentially negative `aggr_idx`.
- [Medium] The metric unit parameter is ignored, and the metric name is erroneously printed as the unit in JSON normal mode.
- [Low] Missing explicit header inclusions for standard POSIX types (`struct timespec` and `bool`), violating musl libc compatibility rules.
--

--- Patch [10]: [PATCH v3 10/14] perf stat: Implement streaming JSON formatting callbacks ---
commit 93c595bb0466fb8a1bacc41beea604151270788e
Author: Ian Rogers <irogers@google.com>

perf stat: Implement streaming JSON formatting callbacks
    
This patch implements streaming JSON formatting callbacks in 
util/stat-print-json.c to optimize JSON output. It bypasses dynamic queues
in favor of streaming objects directly to the output. It uses centralized
aggregation helpers and supports interval-mode timestamps.

> diff --git a/tools/perf/util/stat-print-json.c b/tools/perf/util/stat-print-json.c
> index 4690c4286fd2a..f168dca70ee09 100644
> --- a/tools/perf/util/stat-print-json.c
> +++ b/tools/perf/util/stat-print-json.c
> @@ -1,13 +1,330 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include "stat-print.h"
> +#include <errno.h>
> +#include <inttypes.h>
> +#include <math.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
>  #include <linux/compiler.h>
> +#include <linux/kernel.h>

[Severity: Low]
Does this missing explicit inclusion of <time.h> and <stdbool.h> cause
build failures on musl libc systems? The file uses struct timespec
and bool later on, which require these headers under strict POSIX standard
compliance.

[ ... ]

> +static int json_print_metric(void *ctx, const struct perf_stat_config *config, struct evsel *evsel,
> +			     int aggr_idx, const char *name, const char *unit __maybe_unused,
> +			     double val, enum metric_threshold_classify thresh)
> +{
> +	struct json_print_state *ps = ctx;
> +	FILE *output = config->output;
> +	u64 run = 0, ena = 0;
> +	double enabled_percent = 100.0;
> +	struct perf_stat_evsel *ps_evsel = evsel->stats;
> +
> +	if (ps_evsel && ps_evsel->aggr) {
> +		run = ps_evsel->aggr[aggr_idx].counts.run;
> +		ena = ps_evsel->aggr[aggr_idx].counts.ena;
> +	}

[Severity: High]
Can this cause an out-of-bounds memory read? If perf stat runs with a
global aggregation mode like AGGR_GLOBAL, aggr_idx can be passed as -1.

A later check in this same function confirms that aggr_idx can legitimately
be negative:

    if (config->aggr_map && aggr_idx >= 0) {

Is it possible to check that aggr_idx >= 0 before indexing into
ps_evsel->aggr?

> +
> +	fprintf(output, "{");
> +
> +	/* Print interval timestamp first if configured */
> +	if (config->interval && ps && ps->timestamp[0])
> +		fprintf(output, "%s", ps->timestamp);
> +
> +	/* Print aggregation JSON fields if configured */
> +	if (config->aggr_map && aggr_idx >= 0) {
> +		struct aggr_cpu_id id = config->aggr_map->map[aggr_idx];
> +		int aggr_nr = 0;
> +
> +		if (evsel->stats && evsel->stats->aggr)
> +			aggr_nr = evsel->stats->aggr[aggr_idx].nr;
> +
> +		print_aggr_id_json(config, output, evsel, id, aggr_nr);
> +	}
> +
> +	if (run != ena)
> +		enabled_percent = 100.0 * run / ena;
> +	fprintf(output, "\"event-runtime\" : %" PRIu64 ", \"pcnt-running\" : %.2f", run,
> +		enabled_percent);
> +	fprintf(output, ", \"metric-value\" : \"%f\"", val);
> +	if (name && name[0])
> +		fprintf(output, ", \"metric-unit\" : \"%s\"", name);

[Severity: Medium]
Is this printing the metric name instead of the metric unit?

The function parameter for unit is marked as __maybe_unused, and the
string interpolated for "metric-unit" is the name variable.

> +	if (thresh != METRIC_THRESHOLD_UNKNOWN) {
> +		fprintf(output, ", \"metric-threshold\" : \"%s\"",
> +			metric_threshold_classify__str(thresh));
> +	}
> +	fprintf(output, "}\n");
> +
> +	return 0;
> +}

[ ... ]

> +static int json_metric_only_print_metric(void *ctx,
> +					 const struct perf_stat_config *config __maybe_unused,
> +					 struct evsel *evsel __maybe_unused, int aggr_idx,
> +					 const char *name, const char *unit, double val,
> +					 enum metric_threshold_classify thresh __maybe_unused)
> +{
> +	struct json_metric_only_print_state *ps = ctx;
> +	FILE *output = ps->fp;
> +
> +
> +	if (aggr_idx != ps->last_aggr_idx) {
> +		if (ps->last_aggr_idx != -2)
> +			fprintf(output, "}\n");
> +		fprintf(output, "{");
> +		if (config->interval && ps->timestamp[0])
> +			fprintf(output, "%s", ps->timestamp);
> +		if (config->aggr_map && aggr_idx >= 0) {
> +			struct aggr_cpu_id id = config->aggr_map->map[aggr_idx];
> +			struct evsel *mock_evsel = list_first_entry(&ps->evlist->core.entries,
> +								    struct evsel, core.node);
> +			int aggr_nr = 0;
> +
> +			if (mock_evsel->stats && mock_evsel->stats->aggr)
> +				aggr_nr = mock_evsel->stats->aggr[aggr_idx].nr;
> +
> +			print_aggr_id_json(config, output, mock_evsel, id, aggr_nr);

[Severity: High]
Does this ignore the provided evsel associated with the metric?

This code grabs the first entry in the evlist as a mock_evsel and passes
it to print_aggr_id_json(). If the evlist contains heterogeneous events with
different thread maps or aggregation stat arrays, could accessing them
with the current aggr_idx cause an out-of-bounds read?

> +		}
> +		ps->last_aggr_idx = aggr_idx;
> +		ps->first_in_group = true;
> +	}

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