Re: [PATCH v5 17/58] perf python: Refactor and add accessors to sample event

sashiko-bot
2026-04-24T18:23:37Z

Sashiko AI review found 2 potential issue(s):
- [Critical] Removal of `perf_sample__init()` in `pyrf_event__new()` leaves `pevent->sample` fields uninitialized, leading to an invalid free and crash on error paths.
- [High] Missing `.tp_new` and `.tp_dealloc` handlers for `pyrf_comm_event__type` and `pyrf_read_event__type` causes reference leaks and breaks instantiation.
--

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

perf python: Refactor and add accessors to sample event

This commit refactors event sample types by creating common fields for evsel
and relocating sample-specific fields. It also adds several accessors for
sample events and introduces an address location structure.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index f240905e07d6c1..63ee9bc65721bb 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> @@ -172,6 +218,7 @@ static PyTypeObject pyrf_comm_event__type = {
> .tp_flags = Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE,
> .tp_doc = pyrf_comm_event__doc,
> .tp_members = pyrf_comm_event__members,
> + .tp_getset = pyrf_event__getset,
> .tp_repr = (reprfunc)pyrf_comm_event__repr,
> };

Are .tp_new and .tp_dealloc missing from this structure?

Since the earlier runtime assignments of PyType_GenericNew were removed
during this refactoring, leaving .tp_new NULL breaks direct instantiation
from Python.

Additionally, leaving .tp_dealloc NULL means the Python garbage collector
bypasses the pyrf_event__delete() destructor, so perf_sample__exit() is
never executed.

Could this permanently leak the evsel reference that gets elevated during
evsel__parse_sample() every time a comm event is processed?

[ ... ]

> @@ -269,6 +322,7 @@ static PyTypeObject pyrf_read_event__type = {
> .tp_flags = Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE,
> .tp_doc = pyrf_read_event__doc,
> .tp_members = pyrf_read_event__members,
> + .tp_getset = pyrf_event__getset,
> .tp_repr = (reprfunc)pyrf_read_event__repr,
> };

Similar to the comm event above, does this read event structure also need
.tp_new and .tp_dealloc populated to prevent memory leaks and instantiation
errors?

[ ... ]

> @@ -516,7 +847,9 @@ static PyObject *pyrf_event__new(const union perf_event *event)
> pevent = PyObject_New(struct pyrf_event, ptype);
> if (pevent != NULL) {
> memcpy(&pevent->event, event, event->header.size);
> - perf_sample__init(&pevent->sample, /*all=*/false);
> + pevent->sample.evsel = NULL;
> + pevent->al_resolved = false;
> + addr_location__init(&pevent->al);
> }
> return (PyObject *)pevent;
> }

Can removing perf_sample__init() lead to a crash on error paths?

PyObject_New() allocates memory but does not zero-initialize it.
If evlist__event2evsel() fails to find an event, pyrf_evlist__read_on_cpu()
takes this error path:

pyrf_evlist__read_on_cpu() {
pyevent = pyrf_event__new(event);
...
evsel = evlist__event2evsel(evlist, event);
if (!evsel) {
Py_DECREF(pyevent);
Py_RETURN_NONE;
}
...
}

The Py_DECREF() will invoke the destructor pyrf_event__delete(), which
unconditionally calls perf_sample__exit().

Because perf_sample__init() was removed, the pevent->sample fields like
user_regs and intr_regs might contain uninitialized garbage values.

Will the zfree() calls inside perf_sample__exit() attempt to free invalid
pointers in this scenario?

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