Re: [PATCH v7 10/59] perf evlist: Add reference count

sashiko-bot
2026-04-25T23:17:08Z

Sashiko AI review found 2 potential issue(s):
- [High] Invalid free and heap corruption during garbage collection of Python evlist objects.
- [High] Semantic breakage of `evlist[i]` and potential heap corruption via `evsel__clone()`.
--

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

perf evlist: Add reference count

This commit introduces reference counting for struct evlist to aid in
lifecycle management, especially for the Python bindings. It replaces
evlist__delete() with evlist__put(), which decrements the reference
count and cleans up the object when the count drops to zero.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -1285,15 +1285,22 @@ static int pyrf_evlist__init(struct pyrf_evlist *pevlist,
[ ... ]
> static void pyrf_evlist__delete(struct pyrf_evlist *pevlist)
> {
> - evlist__exit(&pevlist->evlist);
> + evlist__put(pevlist->evlist);
> Py_TYPE(pevlist)->tp_free((PyObject*)pevlist);
> }

Can this lead to an invalid free during garbage collection?

When evlist__put() drops the reference count to zero, it calls
evlist__purge(), which iterates over the events and calls
evsel__delete(). evsel__delete() unconditionally calls free(evsel).

However, for events added via the Python API (evlist.add()), the evsel
pointer passed to the C evlist is an interior pointer embedded directly
within the Python pyrf_evsel object (set up via
evsel = &((struct pyrf_evsel *)pevsel)->evsel;). Calling free() on this
interior pointer would cause heap corruption.

I notice this issue is resolved in a subsequent commit "perf evsel: Add
reference count", which dynamically allocates evsel and implements proper
reference counting.

[ ... ]
> @@ -1786,7 +1793,23 @@ static Py_ssize_t pyrf_evlist__length(PyObject *obj)
> {
[ ... ]
> +static PyObject *pyrf_evsel__from_evsel(struct evsel *evsel)
> +{
> + struct pyrf_evsel *pevsel = PyObject_New(struct pyrf_evsel, &pyrf_evsel__type);
> +
> + if (!pevsel)
> + return NULL;
> +
> + memset(&pevsel->evsel, 0, sizeof(pevsel->evsel));
> + evsel__init(&pevsel->evsel, &evsel->core.attr, evsel->core.idx);
> +
> + evsel__clone(&pevsel->evsel, evsel);
> + if (evsel__is_group_leader(evsel))
> + evsel__set_leader(&pevsel->evsel, &pevsel->evsel);
> + return (PyObject *)pevsel;
> }
>
> static PyObject *pyrf_evlist__item(PyObject *obj, Py_ssize_t i)
> @@ -1794,17 +1817,16 @@ static PyObject *pyrf_evlist__item(PyObject *obj, Py_ssize_t i)
> struct pyrf_evlist *pevlist = (void *)obj;
> struct evsel *pos;
>
> - if (i >= pevlist->evlist.core.nr_entries) {
> + if (i >= pevlist->evlist->core.nr_entries) {
> PyErr_SetString(PyExc_IndexError, "Index out of range");
> return NULL;
> }
>
> - evlist__for_each_entry(&pevlist->evlist, pos) {
> + evlist__for_each_entry(pevlist->evlist, pos) {
> if (i-- == 0)
> break;
> }
> -
> - return Py_BuildValue("O", container_of(pos, struct pyrf_evsel, evsel));
> + return pyrf_evsel__from_evsel(pos);
> }

Does returning a clone of the evsel instead of the original object break
the semantics of evlist[i]?

Python scripts that rely on retrieving and modifying an event inside an
evlist (for example, evsel = evlist[0]; evsel.sample_freq = 100) will now
modify the ephemeral clone, leaving the original event untouched.

Additionally, if evsel__clone() fails internally, its error path calls
evsel__delete(dest) on the provided pointer. Since dest here points to
the interior evsel embedded in the Python object, this would trigger an
invalid free and corrupt the heap.

This semantic breakage and heap corruption issue is also fixed in the
subsequent commit "perf evsel: Add reference count", which removes this
cloning workaround.

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