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

sashiko-bot
2026-04-23T04:24:54Z

Sashiko AI review found 4 potential issue(s):
- [Critical] Type confusion in `pyrf_evlist__add` leads to arbitrary memory write.
- [High] Memory leak of Python `perf.evsel` wrapper objects in `pyrf_evlist__add`.
- [High] Reference count and resource leak for non-sample events in Python API.
- [High] Incomplete mitigation for uninitialized `perf.evsel` objects leading to NULL pointer dereferences.
--

commit 6ccf91b0693b6b304d41b9ebc58803ae64346935
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 3f0758d5bd904..8585ae992e6b0 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);
> }

Is it possible that non-sample events will leak the underlying evsel
reference?

The patch adds evsel__put() to pyrf_sample_event__delete(), but other
event types like pyrf_mmap_event__type or pyrf_task_event__type rely on
the default Python object destructor.

Since pyrf_evlist__read_on_cpu() acquires an evsel reference for all event
types, does this mean non-sample events will permanently leak the reference
when they are garbage collected?

[ ... ]

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

Does this protection fully prevent dereferences of uninitialized evsel
objects?

Since this only implements tp_getattro and not tp_setattro, could property
setters bypass this and directly call C setter functions like
pyrf_evsel__set_tracking() which then dereference pevsel->evsel?

Also, could calling methods directly on the class (e.g., perf.evsel.open(e))
bypass tp_getattro and lead to a NULL dereference?

[ ... ]

> @@ -1566,9 +1728,9 @@ static PyObject *pyrf_evlist__add(struct pyrf_evlist *pevlist,
> return NULL;
>
> Py_INCREF(pevsel);

Since evsel is now dynamically allocated and reference-counted independently,
does this Py_INCREF() cause the Python wrapper object to be leaked?

Before this change, evlist held a pointer to the embedded evsel struct inside
the Python object, making Py_INCREF necessary. Now that evlist__add() takes
its own reference to the standalone C struct via evsel__get(evsel) and never
calls Py_DECREF, shouldn't this Py_INCREF(pevsel) be removed?

> - evsel = &((struct pyrf_evsel *)pevsel)->evsel;
> + evsel = ((struct pyrf_evsel *)pevsel)->evsel;

Does this cast introduce an arbitrary memory write if a different Python
object is passed?

In this function, the user argument is parsed using
PyArg_ParseTuple(args, "O", &pevsel) which accepts any Python object without
type validation.

If a different type (like a bytes object) is passed, this unconditionally
casts it to struct pyrf_evsel *. Now that evsel is a pointer instead of an
embedded struct, the code will read the pointer from the attacker-controlled
object's memory and later execute:

evsel->core.idx = evlist->core.nr_entries;
evlist__add(evlist, evsel__get(evsel));

Could this result in an arbitrary memory write and atomic increment?

Additionally, does this directly access ((struct pyrf_evsel *)pevsel)->evsel
without validating it, potentially bypassing the pyrf_evsel__getattro() check
above and causing a NULL dereference on uninitialized perf.evsel objects?

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

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