Re: [PATCH v3 01/15] perf script: Fix metric_evlist leak in script_find_metrics

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Prematurely setting `metric_evsel->metric_id = NULL` breaks subsequent metric event lookups, null-terminating the `metric_events` array and dropping events.
--

--- Patch [1]: [PATCH v3 01/15] perf script: Fix metric_evlist leak in script_find_metrics ---
commit 5fba9e29ce353d01c2f25b3b58a05d2fc79eb299
Author: Ian Rogers <[email protected]>

perf script: Fix metric_evlist leak in script_find_metrics

Fix a memory leak in script_find_metrics() where metric_evlist is leaked
when returning early on error paths.

> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
> --- a/tools/perf/builtin-script.c
> +++ b/tools/perf/builtin-script.c

[ ... ]

> 	evlist__for_each_entry(metric_evlist, metric_evsel) {
> 		struct evsel *script_evsel =
> 			map_metric_evsel_to_script_evsel(script_evlist, metric_evsel);
> 		struct metric_event *metric_me =
> 			metricgroup__lookup(evlist__metric_events(metric_evlist),
> 					    metric_evsel,
> 					    /*create=*/false);
> 
> 		if (script_evsel->metric_id == NULL) {
> 			script_evsel->metric_id = metric_evsel->metric_id;
> 			metric_evsel->metric_id = NULL;
> 		}

[Severity: High]
This is a pre-existing issue, but could clearing metric_evsel->metric_id
here interfere with mapping resolutions later in this loop?

[ ... ]

> 			list_splice_init(&metric_me->head, &script_me->head);
> 			list_for_each_entry(expr, &script_me->head, nd) {
> 				for (int i = 0; expr->metric_events[i]; i++) {
> 					expr->metric_events[i] =
> 						map_metric_evsel_to_script_evsel(script_evlist,
> 									expr->metric_events[i]);
> 				}
> 			}
> 		}
> 	}

When map_metric_evsel_to_script_evsel() evaluates the required events,
would a NULL metric_id from the earlier transfer cause evsel__metric_id()
to return unknown? 

It looks like this might fail the ID comparison against the script event
and return NULL, which would prematurely terminate the expr->metric_events
array and cause events to be silently dropped.

[ ... ]

> -	evlist__put(metric_evlist);
>  out:
> +	evlist__put(metric_evlist);
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1