Re: [PATCH v3 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:

New issues:
- [High] In metric-only mode, `std_metric_only_print_end` arbitrarily uses the first `evsel` (`mock_evsel`) to format all aggregation IDs, causing potential out-of-bounds accesses.
- [Low] Missing explicit inclusion of `<time.h>`, `<sys/time.h>`, `<sys/resource.h>`, and `<stdbool.h>` for musl compatibility.
- [Low] Negative width parameter in `fprintf` causes massive incorrect padding.

Pre-existing issues:
- [High] Uninitialized stack variable `unit` is passed to metric formatting callbacks.
--

--- Patch [2]: [PATCH v3 02/14] perf stat: Implement standard console (STD) formatting callbacks ---
commit be046d24ddd91343fbf91452501c8366f0f6e34b
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 to replace empty stubs. It introduces
queued_event and queued_metric structures to buffer traversal streams.
It also utilizes centralized aggregation helpers for CPU and thread
prefixes.

> diff --git a/tools/perf/util/stat-print-std.c b/tools/perf/util/stat-print-std.c
> --- a/tools/perf/util/stat-print-std.c
> +++ b/tools/perf/util/stat-print-std.c
> @@ -1,13 +1,828 @@
>  // SPDX-License-Identifier: GPL-2.0
> -#include "stat-print.h"
> +#include <errno.h>
> +#include <math.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +#include <linux/compiler.h>
> +#include <linux/kernel.h>
> +#include <linux/list.h>

[Severity: Low]
Does this code compile on musl libc systems?

The timeval2double() function dereferences struct timeval, and
print_footer_std() accesses struct rusage, but <sys/time.h>,
<sys/resource.h>, <time.h>, and <stdbool.h> are not explicitly included.

[ ... ]

> +static int std_print_metric(void *ctx, const 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;
> +
> +	if (!ps->current_event)
> +		return 0;

[ ... ]

> +	b->name = strdup(name);
> +	if (!b->name) {
> +		free(b);
> +		return -ENOMEM;
> +	}
> +
> +	if (unit && unit[0]) {
> +		b->unit = strdup(unit);

[Severity: High]
This is a pre-existing issue, but does this function occasionally receive
an uninitialized stack pointer?

In tools/perf/util/stat-print.c:calculate_and_print_metric():

    char *unit;
    ...
    if (perf_pmu__convert_scale(metric_unit, &unit, &scale) >= 0)
        ratio *= scale;
    ...
    cb->print_metric(..., unit, ...);

If perf_pmu__convert_scale() fails (e.g., -ENOMEM from parse_double), it
returns a negative value without initializing the unit pointer. This
callback might then call strdup(unit) or dereference unit[0] on
uninitialized stack memory.

[ ... ]

> +static int std_print_end(void *ctx, const struct perf_stat_config *config)
> +{

[ ... ]

> +		/* 2. Print event value (scaled) or spaces if metricgroup */
> +		if (is_metricgroup) {
> +			if (!skip_header) {
> +				int n = fprintf(out, " %*s", EVNAME_LEN, full_name);
> +
> +				fprintf(out, "%*s", MGROUP_LEN + config->unit_width + 2 - n, "");

[Severity: Low]
Can this cause incorrect padding in the console output?

If the metricgroup or PMU name length causes n to be greater than
MGROUP_LEN + config->unit_width + 2, the calculated width passed to
fprintf() will be negative.

[ ... ]

> +static int std_metric_only_print_end(void *ctx, const struct perf_stat_config *config)
> +{

[ ... ]

> +	/* Print values */
> +	list_for_each_entry_safe(b, tmp, &ps->queued_metrics, list) {
> +		if (b->aggr_idx != current_aggr) {
> +			if (current_aggr != -2)
> +				fprintf(out, "\n");
> +			current_aggr = b->aggr_idx;
> +			if (config->interval && ps->timestamp[0])
> +				fprintf(out, "%s", ps->timestamp);
> +			if (config->aggr_map && current_aggr >= 0) {
> +				struct aggr_cpu_id id = config->aggr_map->map[current_aggr];
> +				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[current_aggr].nr;
> +
> +				print_aggr_id_std(config, out, mock_evsel, id, aggr_nr);

[Severity: High]
Could this lead to an out-of-bounds access?

In metric-only mode with AGGR_THREAD aggregation, we arbitrarily use the
first evsel (mock_evsel) to format all aggregation IDs. If the actual
metric was generated by a different event that has more threads than this
system-wide dummy mock_evsel, print_aggr_id_std() could extract an
id.thread_idx that exceeds mock_evsel->core.threads->nr.

[ ... ]

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