Re: [PATCH v1 16/41] perf parse-events: Decode metric-id terms when adding events

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] perf parse-events: memory leaks and lost -EINVAL in __add_event()
--

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

perf parse-events: Decode metric-id terms when adding events

This commit moves metric-id encoding and decoding from metricgroup.c into
parse-events.c so that metric-ids are decoded when evsels are created in
__add_event() rather than rewritten after parsing.

> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 159d847f44ce2..1c02c2b906840 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -171,6 +171,73 @@ static void fix_raw(struct parse_events_terms *config_terms, struct perf_pmu *pm

[ ... ]

> +static int parse_events__decode_metric_id(struct strbuf *sb, const char *x)
> +{
> +	const char *orig = x;
> +	size_t i;
> +	char c;
> +	int ret;
> +
> +	for (; *x; x++) {
> +		c = *x;
> +		if (*x == '!') {
> +			x++;
> +			i = *x - '0';
> +			if (i >= strlen(metric_id_code_characters)) {
> +				pr_err("Bad metric-id encoding in: '%s'", orig);
> +				return -EINVAL;
> +			}
> +			c = metric_id_code_characters[i];
> +		}
> +		ret = strbuf_addch(sb, c);
> +		if (ret)
> +			return ret;
> +	}
> +	return 0;
> +}
> +
>  static struct evsel *
>  __add_event(struct list_head *list, int *idx,
>  	    struct perf_event_attr *attr,
> @@ -261,7 +328,12 @@ __add_event(struct list_head *list, int *idx,
>  	}
>  
>  	if (metric_id) {
> -		evsel->metric_id = strdup(metric_id);
> +		struct strbuf sb = STRBUF_INIT;
> +		int ret = parse_events__decode_metric_id(&sb, metric_id);
> +
> +		if (!ret)
> +			evsel->metric_id = strdup(sb.buf);
> +		strbuf_release(&sb);
>  		if (!evsel->metric_id)
>  			goto out_err;
>  	}

[Severity: Medium]
When parse_events__decode_metric_id() fails on a malformed metric-id
encoding (such as cpu/instructions,metric-id=foo!9/) and returns -EINVAL,
does jumping to out_err in __add_event() lose the -EINVAL error code and
leak both config_terms and evsel->unit?

In __add_event(), out_err frees evsel directly instead of calling
evsel__put(evsel) and returns NULL before config_terms is spliced into
evsel->config_terms:

tools/perf/util/parse-events.c:__add_event() {
	...
	if (config_terms)
		list_splice_init(config_terms, &evsel->config_terms);
	...
out_err:
	perf_cpu_map__put(cpus);
	perf_cpu_map__put(pmu_cpus);
	zfree(&evsel->name);
	zfree(&evsel->metric_id);
	free(evsel);
	return NULL;
}

When __add_event() returns NULL to parse_events_add_pmu() (as well as
add_event() and __parse_events_add_numeric()), the caller converts the
failure into -ENOMEM and returns without freeing config_terms:

tools/perf/util/parse-events.c:parse_events_add_pmu() {
	...
	evsel = __add_event(list, &parse_state->idx, &attr, /*init_attr=*/true,
			    get_config_name(&parsed_terms),
			    get_config_metric_id(&parsed_terms), parse_state->pmus, pmu,
			    &config_terms, first_wildcard_match, term_cpu, alternate_hw_config);
	perf_cpu_map__put(term_cpu);
	if (!evsel) {
		parse_events_terms__exit(&parsed_terms);
		return -ENOMEM;
	}
	...
}

Previously, metric-id decoding ran after __add_event() had already spliced
config_terms into evsel->config_terms, and a failure in
decode_all_metric_ids() cleaned up the evlist via evlist__delete() ->
evsel__delete() -> evsel__exit().

Note that a later patch in the series, commit 21ed6e9ff56d ("perf evsel:
Generalize metric_id to a set of metric_ids"), replaces free(evsel) with
evsel__put(evsel) in __add_event(), which resolves the evsel->unit leak,
but the unspliced config_terms leak in parse_events_add_pmu() and the loss
of the -EINVAL return code remain.

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