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

sashiko-bot
2026-04-25T23:33:53Z

Sashiko AI review found 5 potential issue(s):
- [High] Incorrect minimum size validation for mmap events breaks parsing.
- [High] Missing `.tp_dealloc` in `comm_event` and `read_event` causes `evsel` reference leaks.
- [High] Dereferencing a NULL `evsel` pointer in `is_tracepoint()` when accessing attributes of a manually instantiated `sample_event`.
- [High] The Python extension supports PERF_RECORD_MMAP but lacks support for PERF_RECORD_MMAP2, violating subsystem guidelines.
- [High] Python's `T_STRING_INPLACE` modifier reads past the dynamically allocated object boundary if string fields lack a null terminator.
--

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

perf python: Refactor and add accessors to sample event

Add common evsel field for events and move sample specific fields to
only be present in sample events. Add accessors for sample events.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 861973144106..824cf58645e0 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,
> };

[ ... ]

> @@ -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,
> };

Are we missing a .tp_dealloc assignment for pyrf_comm_event__type and
pyrf_read_event__type?

Since pyrf_event__new() now calls evsel__parse_sample() for all events,
which acquires an evsel reference, does this cause an evsel memory leak when
Python garbage collects these events because perf_sample__exit() is not
called?

[ ... ]

> @@ -373,6 +428,199 @@ get_tracepoint_field(struct pyrf_event *pevent, PyObject *attr_name)
> }
> #endif /* HAVE_LIBTRACEEVENT */
>

[ ... ]

> +static PyObject*
> +pyrf_sample_event__getattro(struct pyrf_event *pevent, PyObject *attr_name)
> +{
> + PyObject *obj = NULL;
> +
> +#ifdef HAVE_LIBTRACEEVENT
> + if (is_tracepoint(pevent))
> + obj = get_tracepoint_field(pevent, attr_name);
> +#endif
> +
> + return obj ?: PyObject_GenericGetAttr((PyObject *) pevent, attr_name);
> +}

Can pevent->sample.evsel be NULL here if the event is manually instantiated
from Python?

If so, does is_tracepoint() dereference a NULL evsel pointer?

[ ... ]

> @@ -490,10 +821,14 @@ static PyTypeObject *pyrf_event__type[] = {

[ ... ]

> -static PyObject *pyrf_event__new(const union perf_event *event)
> +static PyObject *pyrf_event__new(const union perf_event *event, struct evsel *evsel,
> + struct perf_session *session __maybe_unused)
> {
> struct pyrf_event *pevent;
> PyTypeObject *ptype;
> + size_t size;
> + int err;
> + size_t min_size = sizeof(struct perf_event_header);
>
> if ((event->header.type < PERF_RECORD_MMAP ||
> event->header.type > PERF_RECORD_SAMPLE) &&
> @@ -504,19 +839,62 @@ static PyObject *pyrf_event__new(const union perf_event *event)
> return NULL;
> }

Does this code need to handle PERF_RECORD_MMAP2?

The perf tools guidelines state that tools handling MMAP must also handle
MMAP2, but it appears MMAP2 events would be rejected here and raise a
TypeError since they fall outside the accepted range.

[ ... ]

> + switch (event->header.type) {
> + case PERF_RECORD_MMAP:
> + min_size = sizeof(struct perf_record_mmap);
> + break;

[ ... ]

> + if (event->header.size < min_size)
> + return PyErr_Format(PyExc_ValueError, "Event size %u too small for type %u",
> + event->header.size, event->header.type);

Does sizeof(struct perf_record_mmap) statically include the full char
filename[PATH_MAX] array?

Since the kernel dynamically sizes mmap events to only include the actual
string length plus padding, will this check evaluate to true and
unconditionally reject valid mmap events?

[ ... ]

> + /* Allocate just enough memory for the size of event. */
> + size = offsetof(struct pyrf_event, event) + event->header.size;
> + pevent = (struct pyrf_event *)PyObject_Malloc(size);

Could this exact sized allocation lead to out of bounds reads?

If a malformed event has a header size that exactly covers the text of a
string but omits the null terminator, Python's T_STRING_INPLACE handler
might use strlen() and read past the end of the allocated object into
adjacent heap memory.

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