Re: [PATCH v1 15/26] perf expr: Use generic hashmap for expr_ids

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Medium] perf metricgroup: memory leak on realloc() failure in
  resolve_metric() [1]
--

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

perf expr: Use generic hashmap for expr_ids

This commit converts expr_parse_ctx::ids and its helper functions to use a
typed hashmap declared with DECLARE_HASHMAP(expr_ids_map, const char *,
struct expr_id_data *).

> diff --git a/tools/perf/util/metricgroup.c b/tools/perf/util/metricgroup.c
> index 779fe56fe8d0c..41887fad49682 100644
> --- a/tools/perf/util/metricgroup.c
> +++ b/tools/perf/util/metricgroup.c

[ ... ]

> @@ -765,10 +765,10 @@ static int resolve_metric(struct list_head *metric_list,
>  	 * Iterate all the parsed IDs and if there's a matching metric and it to
>  	 * the pending array.
>  	 */
> -	hashmap__for_each_entry(root_metric->pctx->ids, cur, bkt) {
> +	hashmap__for_each(root_metric->pctx->ids, key, val) {
>  		struct pmu_metric pm;
>  
> -		if (pmu_metrics_table__find_metric(table, pmu, cur->pkey,
> +		if (pmu_metrics_table__find_metric(table, pmu, key,
>  						   metricgroup__find_metric_callback,
>  						   &pm) != PMU_METRICS__NOT_FOUND) {
>  			pending = realloc(pending,
> @@ -777,7 +777,7 @@ static int resolve_metric(struct list_head *metric_list,
>  				return -ENOMEM;
>  
>  			memcpy(&pending[pending_cnt].pm, &pm, sizeof(pm));
> -			pending[pending_cnt].key = cur->pkey;
> +			pending[pending_cnt].key = key;
>  			pending_cnt++;
>  		}
>  	}

[Severity: Medium]
This problem wasn't introduced by this patch, but can a realloc() failure
in resolve_metric() leak the existing pending buffer [1]?

When resolving a metric that references multiple sub-metrics, pending is
grown one entry at a time; assigning the result of realloc() directly back
to pending overwrites the prior pointer with NULL on failure and returns
-ENOMEM without freeing it.  __add_metric() has the same issue when growing
root_metric->metric_refs with realloc().

[1] https://sashiko.dev/#/bug/linux-ca3710c6-abe9-465b-8da4-8cb1ea1a2b6b

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