Re: [PATCH v5 10/58] perf evlist: Add reference count

sashiko-bot
2026-04-24T17:25:03Z

Sashiko AI review found 2 potential issue(s):
- [High] Python API crash (BUG_ON) and semantic breakage in `evlist[i]` due to dynamic cloning.
- [High] Heap corruption due to invalid `free()` of an interior Python object pointer during `evlist` destruction.
--

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

perf evlist: Add reference count

This commit introduces a reference count for the evlist to prevent issues
with cloning in the python code. It switches from evlist__delete() to
a refcounted evlist__put() approach.

> The combination is needed for the python code to operate
> correctly (not hit asserts in the evsel clone), but the changes are
> broken apart for the sake of smaller patches.

Does this intentionally break bisectability? Even though the fix is
introduced in the next patch ("perf evsel: Add reference count"), leaving
intermediate commits broken makes bisection difficult.

> 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);
> }

Does this trigger an invalid free when destroying the evlist? Calling
evlist__put() will drop the refcount and eventually call evlist__purge(),
which iterates over elements and calls evsel__delete() (which calls free).

However, when an evsel is added from Python via pyrf_evlist__add():

tools/perf/util/python.c:pyrf_evlist__add() {
...
evsel = &((struct pyrf_evsel *)pevsel)->evsel;
evlist__add(evlist, evsel);
...
}

The evsel is an interior pointer embedded within a Python-allocated
pyrf_evsel object. Passing this interior pointer to free() will corrupt
the heap.

[ ... ]
> @@ -1794,17 +1817,16 @@ static PyObject *pyrf_evlist__item(PyObject *obj, Py_ssize_t i)
[ ... ]
> - return Py_BuildValue("O", container_of(pos, struct pyrf_evsel, evsel));
> + return pyrf_evsel__from_evsel(pos);
> }

Can this cause crashes and semantic changes for python scripts? The new
pyrf_evsel__from_evsel() dynamically clones the evsel using evsel__clone().

If a script calls evlist.open() before iterating, the underlying
evsel__clone() hits a BUG_ON(orig->core.fd) and crashes. Also, returning a
detached clone means any modifications made to the event by the Python
script are discarded and do not affect the actual event in the list.

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