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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Out-of-bounds array access and NULL pointer dereference when printing aggregation headers.
- [Low] Uninitialized struct fields in dynamically allocated memory.
- [Low] Redundant and inverted multiplexing scaling logic acts as dead code.
--

--- Patch [2]: [PATCH v4 02/14] perf stat: Implement standard console (STD) formatting callbacks ---
commit 42fd504b1ec9248fe9557a917da5fa2928b1bb6b
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, replacing the empty stubs introduced in Commit 1.
Introduces the format-private queued_event and queued_metric DOM nodes to
buffer traversal streams.

> diff --git a/tools/perf/util/stat-print-std.c b/tools/perf/util/stat-print-std.c
> index a1311f950559b..d83410ae3385a 100644
> --- a/tools/perf/util/stat-print-std.c
> +++ b/tools/perf/util/stat-print-std.c
[ ... ]
> +static int std_print_event(void *ctx, struct perf_stat_config *config,
> +			   struct evsel *evsel, int aggr_idx, u64 val, u64 ena,
> +			   u64 run, double stdev_pct, const char *cgrp)
> +{
> +	struct std_print_state *ps = ctx;
> +	struct queued_event *ev;
[ ... ]
> +	ev = malloc(sizeof(*ev));
> +	if (!ev)
> +		return -ENOMEM;
> +
> +	ev->name = strdup(evsel__name(evsel));

[Severity: Low]
Are all fields in struct queued_event initialized after allocation? It looks
like ev->is_metricgroup remains uninitialized here.

[ ... ]
> +static int std_print_metric(void *ctx, struct perf_stat_config *config __maybe_unused,
> +			    struct evsel *evsel __maybe_unused, int aggr_idx __maybe_unused,
> +			    const char *name, const char *unit, double val,
> +			    enum metric_threshold_classify thresh)
> +{
> +	struct std_print_state *ps = ctx;
> +	struct queued_metric *b;
[ ... ]
> +	b = malloc(sizeof(*b));
> +	if (!b)
> +		return -ENOMEM;
> +
> +	b->name = strdup(name);

[Severity: Low]
Similarly, are all fields in struct queued_metric initialized? It appears
b->evsel and b->aggr_idx remain uninitialized here.

[ ... ]
> +static int std_print_end(void *ctx, struct perf_stat_config *config)
> +{
[ ... ]
> +			if (ev->run == 0 || ev->ena == 0) {
> +				fprintf(out, "%*s ", COUNTS_LEN, bad_count);
> +			} else {
> +				double scaled = (double)ev->val;
> +				double avg;
> +
> +				if (ev->ena < ev->run)
> +					scaled = (double)ev->val * ev->run / ev->ena;

[Severity: Low]
Is this scaling logic correct?

It looks like the condition ev->ena < ev->run is inverted, as an event cannot
physically run longer than it was enabled, making this dead code.

Additionally, if the condition were corrected, wouldn't this double-scale the
value, as ev->val is already scaled by perf_counts_values__scale() during
counter processing?

[ ... ]
> +static int std_metric_only_print_end(void *ctx, struct perf_stat_config *config)
> +{
[ ... ]
> +		if (config->aggr_map && first_aggr >= 0 &&
> +		    config->aggr_mode != AGGR_GLOBAL) {
> +			int len = aggr_header_lens[config->aggr_mode];

[Severity: High]
Can a corrupted or maliciously crafted perf.data file provide an arbitrary
config->aggr_mode here?

Since perf_event__read_stat_config() directly assigns the value to
stat_config.aggr_mode without validation, indexing aggr_header_lens with
it could lead to an out-of-bounds read or NULL pointer dereference when
processing untrusted input.

[ ... ]
> +int perf_stat__print_std(struct evlist *evlist, struct perf_stat_config *config,
> +			 const struct target *target, const struct timespec *ts,
> +			 int argc, const char **argv)
> +{
[ ... ]
> +	} else {
> +		if (config->interval && !config->headers_printed) {
> +			FILE *output = config->output;
> +
> +			if (config->aggr_mode == AGGR_GLOBAL) {
> +				fprintf(output, "#%*s %*s %*s events\n",
> +					15 - 2, "time", 18, "counts",
> +					config->unit_width, "unit");
> +			} else {
> +				fprintf(output,
> +					"#%*s %-*s ctrs %*s %*s events\n",
> +					15 - 2, "time",
> +					aggr_header_lens[config->aggr_mode],
> +					aggr_header_std[config->aggr_mode],
> +					18, "counts", config->unit_width,
> +					"unit");
> +			}

[Severity: High]
Could the same out-of-bounds indexing occur here with config->aggr_mode?

Unvalidated indexing into statically sized arrays for interval headers might
trigger a crash if aggr_mode exceeds the array bounds.

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