Re: [PATCH v1 11/58] perf evsel: Add reference count

sashiko-bot
2026-04-20T00:48:17Z

Sashiko AI review found 6 potential issue(s):
- [High] Missing `evsel__get()` in `pyrf_evlist__read_on_cpu` causes a Use-After-Free of the `evsel` structure.
- [High] Uninitialized pointer dereference in the `pyrf_evlist__read_on_cpu` error path causes a crash.
- [High] Memory leak of the Python wrapper and Use-After-Free of the C `evsel` in `pyrf_evlist__add`.
- [Medium] NULL pointer dereference in `perf.evsel` property getters and setters crashes the interpreter.
- [Medium] Memory leak of the `evsel` structure upon multiple calls to `perf.evsel.__init__`.
- [Low] Python C API violation in `pyrf_evsel__init` returns an error without setting an exception.
--

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

perf evsel: Add reference count

As with evlist this a no-op for most of the perf tool. The reference
count is set to 1 at allocation, the put will see the 1, decrement it
and perform the delete. The purpose for adding the reference count is
for the python code. Prior to this change the python code would clone
evsels, but this has issues if events are opened, etc. leading to
assertion failures. With a reference count the same evsel can be used
and the reference count incremented for the python usage. To not
change the python evsel API getset functions are added for the evsel
members, no set function is provided for size as it doesn't make sense
to alter this.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 9878547e98d6f..66093f7c753db 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -274,6 +274,7 @@ static PyMemberDef pyrf_sample_event__members[] = {
>
> static void pyrf_sample_event__delete(struct pyrf_event *pevent)
> {
> + evsel__put(pevent->evsel);
> perf_sample__exit(&pevent->sample);
> Py_TYPE(pevent)->tp_free((PyObject*)pevent);
> }

With this new evsel__put() added, could this cause a use-after-free regression
or a crash when handling errors in pyrf_evlist__read_on_cpu()?

In pyrf_evlist__read_on_cpu(), the event object is allocated without
zero-initialization, and evsel is assigned later:

PyObject *pyevent = pyrf_event__new(event);
struct pyrf_event *pevent = (struct pyrf_event *)pyevent;
struct evsel *evsel;

if (pyevent == NULL)
return PyErr_NoMemory();

evsel = evlist__event2evsel(evlist, event);
if (!evsel) {
Py_DECREF(pyevent);
Py_INCREF(Py_None);
return Py_None;
}

pevent->evsel = evsel;

If evlist__event2evsel() returns NULL, Py_DECREF(pyevent) is called. Since
pevent->evsel is uninitialized, won't pyrf_sample_event__delete() pass a
garbage pointer to evsel__put() and crash?

Additionally, if evsel is successfully assigned, pyrf_evlist__read_on_cpu()
sets pevent->evsel = evsel without incrementing the reference count via
evsel__get(). When this Python event is garbage collected, the unconditionally
called evsel__put() will decrement the reference count owned by the evlist.
Could this regression lead to premature freeing of the evsel and a subsequent
use-after-free when the evlist tries to access it?

[ ... ]

> @@ -1053,20 +1054,20 @@ static int pyrf_evsel__init(struct pyrf_evsel *pevsel,
> attr.sample_id_all = sample_id_all;
> attr.size = sizeof(attr);
>
> - evsel__init(&pevsel->evsel, &attr, idx);
> - return 0;
> + pevsel->evsel = evsel__new(&attr);
> + return pevsel->evsel ? 0 : -1;
> }

Since __init__ can be manually invoked multiple times in Python, could this
cause a memory leak regression? If a user calls __init__ again on an existing
object, the previously allocated evsel pointer in pevsel->evsel will be
overwritten without calling evsel__put(), leaking the evsel memory.

Also, if evsel__new(&attr) fails and returns NULL, this function evaluates to
-1 and returns. Is it required to set a Python exception like PyErr_NoMemory()
before returning -1 here? Returning an error without setting an exception
violates the Python C API contract.

[ ... ]

> @@ -1225,26 +1226,170 @@ static PyMethodDef pyrf_evsel__methods[] = {
> { .ml_name = NULL, }
> };

[ ... ]

> +static PyObject *pyrf_evsel__get_attr_type(PyObject *self, void */*closure*/)
> +{
> + struct pyrf_evsel *pevsel = (void *)self;
> +
> + return PyLong_FromUnsignedLong(pevsel->evsel->core.attr.type);
> +}

Could this lead to a NULL pointer dereference regression? The pyrf_evsel__type
uses PyType_GenericNew for its .tp_new slot, which allows a user to
instantiate an object directly via perf.evsel.__new__(perf.evsel).

This allocates the Python object with zeroed memory, leaving pevsel->evsel as
NULL. If __init__ is bypassed, any access to the object's properties (such as
.type calling this function) will unconditionally dereference the NULL
pevsel->evsel and crash the interpreter.

[ ... ]

> @@ -1565,7 +1710,7 @@ static PyObject *pyrf_evlist__add(struct pyrf_evlist *pevlist,
> return NULL;
>
> Py_INCREF(pevsel);
> - evsel = &((struct pyrf_evsel *)pevsel)->evsel;
> + evsel = ((struct pyrf_evsel *)pevsel)->evsel;
> evsel->core.idx = evlist->core.nr_entries;
> evlist__add(evlist, evsel);

Could this lead to a memory leak of the Python wrapper and a use-after-free
regression of the C evsel? The Python object's reference count is
intentionally incremented via Py_INCREF(pevsel) to keep it alive, but the
inner evsel is added to the evlist without calling evsel__get().

When the evlist is purged, it will call evsel__put(), which drops the
reference count and frees the evsel structure. Won't the permanently leaked
Python wrapper then hold a dangling pointer, leading to a use-after-free if
its attributes are accessed?

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