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

sashiko-bot
2026-04-24T17:31:29Z

Sashiko AI review found 4 potential issue(s):
- [High] Missing `tp_setattro` handler for the `perf.evsel` Python object causes interpreter crashes on uninitialized attribute assignments.
- [High] Type confusion and arbitrary memory write in `pyrf_evlist__add` due to `evsel` pointer dereference.
- [High] Calling `str()` or `repr()` on an uninitialized `perf.evsel` Python object crashes due to a NULL pointer dereference.
- [Medium] The `pyrf_evlist__add` function unconditionally leaks the `pevsel` Python wrapper object every time an event is added.
--

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

perf evsel: Add reference count

This commit introduces a reference count to struct evsel to better handle
its lifecycle, particularly in the Python bindings, where events were
previously cloned and caused assertion failures. Get and set functions are
added for Python evsel members to maintain the API.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 3f0758d5bd9041..8585ae992e6b03 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -1192,7 +1200,7 @@ static PyObject *pyrf_evsel__read(struct pyrf_evsel *pevsel,
> static PyObject *pyrf_evsel__str(PyObject *self)
> {
> struct pyrf_evsel *pevsel = (void *)self;
> - struct evsel *evsel = &pevsel->evsel;
> + struct evsel *evsel = pevsel->evsel;
>
> return PyUnicode_FromFormat("evsel(%s/%s/)", evsel__pmu_name(evsel), evsel__name(evsel));
> }

Since pevsel->evsel is now a dynamically allocated pointer, it can be NULL
if the Python object is instantiated without a successful initialization.

Could this lead to a NULL pointer dereference when str() or repr() is
called on an uninitialized perf.evsel object, since evsel__pmu_name(evsel)
will attempt to access the NULL pointer?

[ ... ]
> @@ -1225,30 +1233,183 @@ static PyMethodDef pyrf_evsel__methods[] = {
> { .ml_name = NULL, }
> };

[ ... ]

> +static PyObject *pyrf_evsel__getattro(struct pyrf_evsel *pevsel, PyObject *attr_name)
> +{
> + if (!pevsel->evsel) {
> + PyErr_SetString(PyExc_ValueError, "evsel not initialized");
> + return NULL;
> + }
> + return PyObject_GenericGetAttr((PyObject *) pevsel, attr_name);
> +}
> +
> static PyTypeObject pyrf_evsel__type = {
> PyVarObject_HEAD_INIT(NULL, 0)
> .tp_name = "perf.evsel",
> @@ -1256,11 +1417,12 @@ static PyTypeObject pyrf_evsel__type = {
> .tp_dealloc = (destructor)pyrf_evsel__delete,
> .tp_flags = Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE,
> .tp_doc = pyrf_evsel__doc,
> - .tp_members = pyrf_evsel__members,
> + .tp_getset = pyrf_evsel__getset,
> .tp_methods = pyrf_evsel__methods,
> .tp_init = (initproc)pyrf_evsel__init,
> .tp_str = pyrf_evsel__str,
> .tp_repr = pyrf_evsel__str,
> + .tp_getattro = (getattrofunc) pyrf_evsel__getattro,
> };

While tp_getattro is provided to protect property reads on uninitialized
objects, tp_setattro is not defined.

Does this mean Python falls back to PyObject_GenericSetAttr, which will
execute C setters like pyrf_evsel__set_tracking directly?

If so, could this cause a segmentation fault in the Python interpreter if
properties are written to an uninitialized perf.evsel object, since the
setters unconditionally dereference pevsel->evsel?

[ ... ]
> @@ -1566,9 +1728,9 @@ 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);
> + evlist__add(evlist, evsel__get(evsel));
>
> return Py_BuildValue("i", evlist->core.nr_entries);
> }

Because pevsel->evsel is now a pointer instead of an embedded struct, does
the assignment to evsel->core.idx introduce a potential arbitrary memory
write if pevsel is passed as a different object type, given that
PyArg_ParseTuple uses the "O" format without type validation?

Additionally, if an uninitialized perf.evsel object is passed, evsel will
be NULL. Could this lead to an immediate NULL pointer dereference when
writing to evsel->core.idx?

Finally, since evlist now manages the evsel lifecycle via evsel__get(evsel)
and the evsel struct is separated from the Python wrapper, is the
Py_INCREF(pevsel) call still necessary?

Because the Python wrapper object is no longer stored in the C structures,
will leaving Py_INCREF(pevsel) cause a memory leak of the Python object each
time an event is added?

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