Re: [PATCH v3 04/15] perf python: Fix memory leak in pyrf_evlist__get_pollfd

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but missing NULL check on the return value of `PyLong_FromLong` in `pyrf_counts_values_get_values` leads to a crash.
- [High] This problem wasn't introduced by this patch, but there is a memory leak of `key` and `value` objects on the success path of `PyDict_SetItem` in `pyrf__metrics_cb`.
--

--- Patch [4]: [PATCH v3 04/15] perf python: Fix memory leak in pyrf_evlist__get_pollfd ---
commit 4d7f49a1892b79c6131283be18ebf7cc1b0fd24f
Author: Ian Rogers <[email protected]>

perf python: Fix memory leak in pyrf_evlist__get_pollfd

This patch fixes a Python list object leak in pyrf_evlist__get_pollfd() by
adding a missing Py_DECREF on an error exit path. It also adds a check
for list allocation failure.

Assisted-by: Antigravity:gemini-3.1-pro
Fixes: 877108e42b1b ("perf tools: Initial python binding")
Signed-off-by: Ian Rogers <[email protected]>

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index af85c0771c448..c7ae1f85aec6d 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -2825,6 +2825,8 @@ static PyObject *pyrf_evlist__get_pollfd(struct pyrf_evlist *pevlist,
>  
>  	evlist = pevlist->evlist;
>  	list = PyList_New(0);
> +	if (!list)
> +		return NULL;
>  
>  	for (i = 0; i < evlist__core(evlist)->pollfd.nr; ++i) {
>  		PyObject *file;
> @@ -2843,6 +2845,7 @@ static PyObject *pyrf_evlist__get_pollfd(struct pyrf_evlist *pevlist,
>  
>  	return list;
>  free_list:
> +	Py_XDECREF(list);
>  	return PyErr_NoMemory();
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential crash
here in pyrf_counts_values_get_values() when memory is exhausted:

tools/perf/util/python.c:pyrf_counts_values_get_values() {
	...
	PyObject *vals = PyList_New(5);

	if (!vals)
		return NULL;
	for (int i = 0; i < 5; i++)
		PyList_SetItem(vals, i, PyLong_FromLong(self->values.values[i]));

	return vals;
}

Can PyLong_FromLong() return a null pointer during memory pressure?

If so, PyList_SetItem() will place the null pointer directly into the list's
internal array without validation.

Could this cause operations like PyObject_Repr to later dereference the
pointer and crash the Python interpreter abruptly instead of raising a
MemoryError?


[Severity: High]
This is a pre-existing issue, but there appears to be a memory leak in
pyrf__metrics_cb():

tools/perf/util/python.c:pyrf__metrics_cb() {
	...
	PyObject *key = dict ? PyUnicode_FromString("MetricGroup") : NULL;
	PyObject *value = key ? pyrf__metrics_groups(pm) : NULL;

	if (!value || PyDict_SetItem(dict, key, value) != 0) {
		Py_XDECREF(key);
		Py_XDECREF(value);
		Py_XDECREF(dict);
		return -ENOMEM;
	}

	if (!add_to_dict(dict, "MetricName", pm->metric_name) ||
	...
}

Does the success path of PyDict_SetItem leak the key and value objects?

The error path correctly drops the references, but if PyDict_SetItem
succeeds, it does not steal the references.

Since this function is called as a callback in pmu_for_each_sys_metric(),
will this unconditionally leak both objects for every valid system metric
processed?

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