Re: [PATCH v2 14/16] perf python: Fix count_values memory leak in pyrf_evsel__read

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A partially failed allocation in `evsel__ensure_counts()` results in a NULL pointer dereference when retrying `pyrf_evsel__read()`.
- [High] The `evsel->prev_raw_counts` array allocated for Python `perf.evsel` objects is unconditionally leaked when the object is destroyed.
- [Medium] String duplication leak in `prepare_metric` on error paths.
--

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

perf python: Fix count_values memory leak in pyrf_evsel__read

In pyrf_evsel__read, if PyArg_ParseTuple fails, the allocated count_values
is leaked. Move the allocation of count_values after the PyArg_ParseTuple
call to prevent the memory leak.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index b17b191b16255..e76a407cb2bb9 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -2204,11 +2204,6 @@ static PyObject *pyrf_evsel__read(struct pyrf_evsel *pevsel,
>  
>  	CHECK_INITIALIZED(evsel, "evsel");
>  
> -	count_values = PyObject_New(struct pyrf_counts_values,
> -							       &pyrf_counts_values__type);
> -	if (!count_values)
> -		return NULL;
> -
>  	if (!PyArg_ParseTuple(args, "ii", &cpu, &thread))
>  		return NULL;
>  
> @@ -2227,6 +2222,10 @@ static PyObject *pyrf_evsel__read(struct pyrf_evsel *pevsel,
>  	if (evsel__ensure_counts(evsel))
>  		return PyErr_NoMemory();
>  
> +	count_values = PyObject_New(struct pyrf_counts_values, &pyrf_counts_values__type);
> +	if (!count_values)
> +		return NULL;
> +
>  	/* Set up pointers to the old and newly read counter values. */
>  	old_count = perf_counts(evsel->prev_raw_counts, cpu_idx, thread_idx);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential NULL
pointer dereference here.

If evsel__ensure_counts() successfully allocates evsel->counts but fails to
allocate evsel->prev_raw_counts, it returns -ENOMEM. If a user script
handles the resulting Python MemoryError and retries the read operation,
evsel__ensure_counts() will see that evsel->counts is no longer NULL and
return success immediately, without allocating evsel->prev_raw_counts.

Will this cause perf_counts() to dereference a NULL prev_raw_counts pointer
on the retry?

[Severity: High]
This is also a pre-existing issue, but does evsel->prev_raw_counts leak
when the object is destroyed?

When a standalone Python evsel object is destroyed, its destruction path
goes through pyrf_evsel__delete() -> evsel__put() -> evsel__exit(). While
evsel__exit() properly calls evsel__free_counts() to free evsel->counts,
it seems to miss freeing evsel->prev_raw_counts.

Could this unconditionally drain memory if Python evsel objects are used in
a loop?

>  	new_count = perf_counts(evsel->counts, cpu_idx, thread_idx);

[ ... ]

[Severity: Medium]
This is another pre-existing issue not caused by this patch, but looking at
prepare_metric() in the same file, does this error path leak the duplicated
string n?

tools/perf/util/python.c:prepare_metric() {
    ...
    char *n = strdup(evsel__metric_id(cur));

    if (!n)
        return -ENOMEM;
    ...
    ret = evsel__ensure_counts(cur);
    if (ret)
        return ret;
    ...
    ret = expr__add_id_val_source_count(pctx, n, val, source_count);
    if (ret)
        return ret;
    ...
}

If evsel__ensure_counts() or expr__add_id_val_source_count() returns an
error, the function returns early without freeing n. Should we ensure n is
freed on these error paths?

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