Re: [RFC PATCH v1 01/14] perf stat: Introduce core generic print traversal engine and header stubs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Unconditional invocation of `cb->print_metric` without verifying the callback pointer.
--

commit 90d8e5db3452e883fc9b511756f3c3b819d1ad40
Author: Ian Rogers <irogers@google.com>

perf stat: Introduce core generic print traversal engine and header stubs

This commit introduces the initial infrastructure for decoupling the perf stat
printing API. It declares the struct perf_stat_print_callbacks interface and
implements the core traversal driver perf_stat__print_cb().

> diff --git a/tools/perf/util/stat-print.c b/tools/perf/util/stat-print.c
> index 0000000000000..3a634396c7c63
> --- /dev/null
> +++ b/tools/perf/util/stat-print.c
[ ... ]
> +static int calculate_and_print_metric(const struct perf_stat_config *config,
> +				      const struct perf_stat_print_callbacks *cb, void *outer_ctx,
> +				      struct metric_expr *mexp, struct evsel *evsel, int aggr_idx)
> +{
[ ... ]
> +	if (!metric_events[i]) {
> +		if (expr__parse(&ratio, pctx, metric_expr) == 0) {
> +			char *unit;
> +
> +			if (metric_threshold &&
> +			    expr__parse(&threshold, pctx, metric_threshold) == 0 &&
> +			    !isnan(threshold)) {
> +				thresh = fpclassify(threshold) == FP_ZERO ? METRIC_THRESHOLD_GOOD :
> +									    METRIC_THRESHOLD_BAD;
> +			}
> +
> +			if (metric_unit && metric_name) {
> +				if (perf_pmu__convert_scale(metric_unit, &unit, &scale) >= 0) {
> +					ratio *= scale;
> +				}
> +				ret = cb->print_metric(outer_ctx, config, evsel, aggr_idx,
> +						       metric_name, unit, ratio, thresh);
> +			} else {
> +				ret = cb->print_metric(outer_ctx, config, evsel, aggr_idx,
> +						       metric_name ?: (evsel->name ?: ""), NULL,
> +						       ratio, thresh);
> +			}

[Severity: Medium]
Could this cause a NULL pointer dereference if a caller implements the new API
but omits the print_metric callback?

Looking at perf_stat__print_cb(), callbacks like print_start, print_event,
and print_end are explicitly checked before invocation, which implies that
the callback pointers are treated as optional:

> +int perf_stat__print_cb(struct evlist *evlist, const struct perf_stat_config *config,
[ ... ]
> +	if (cb->print_start) {
> +		ret = cb->print_start(ctx, config);
> +		if (ret)
> +			return ret;
> +	}
[ ... ]
> +					if (cb->print_event) {
> +						double stdev_pct = 0.0;
[ ... ]
> +						ret = cb->print_event(ctx, config, counter,
> +								      aggr_idx, val, ena, run,
> +								      stdev_pct);

Should print_metric be conditionally checked in calculate_and_print_metric() as
well to ensure safety if it is left unpopulated?

> +		}
> +	}
> +
> +	expr__ctx_free(pctx);
> +	return ret;
> +}

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