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

sashiko-bot
2026-04-25T18:43:34Z

Sashiko AI review found 4 potential issue(s):
- [Critical] Dynamically allocating `pyrf_event` based on `event->header.size` while exposing fixed-offset `PyMemberDef` attributes causes a heap out-of-bounds read/write vulnerability and violates the Python C API.
- [High] Missing `.tp_dealloc` initialization for `pyrf_comm_event__type` and `pyrf_read_event__type` leads to memory and reference leaks.
- [High] Unsafe dereference of `map__dso()` leads to a NULL pointer dereference in `pyrf_sample_event__get_dso`, `pyrf_sample_event__get_dso_long_name`, and `pyrf_sample_event__get_dso_bid`.
- [Medium] Address truncation of 64-bit map boundaries on 32-bit architectures in `pyrf_sample_event__get_map_start` and `pyrf_sample_event__get_map_end`.
--

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

perf python: Refactor and add accessors to sample event

This commit refactors the Python perf module by adding a common evsel
field for events, moving sample specific fields to only be present in
sample events, and adding accessors for sample events.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index a10c4a292756..0de3c657d023 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,
> };

It looks like .tp_dealloc and .tp_new are not initialized for
pyrf_comm_event__type or pyrf_read_event__type in this patch.

Since pyrf_event__new() now unconditionally calls evsel__parse_sample(),
which populates the sample and grabs an evsel reference, will this leak
memory and evsel references when the Python garbage collector cleans up
COMM and READ events?

[ ... ]
> @@ -373,6 +428,199 @@ get_tracepoint_field(struct pyrf_event *pevent, PyObject *attr_name)
> }
> #endif /* HAVE_LIBTRACEEVENT */
>
> +static int pyrf_sample_event__resolve_al(struct pyrf_event *pevent)
> +{
[ ... ]
> +static PyObject *pyrf_sample_event__get_dso(struct pyrf_event *pevent,
> + void *closure __maybe_unused)
> +{
> + if (pyrf_sample_event__resolve_al(pevent) < 0 || !pevent->al.map)
> + Py_RETURN_NONE;
> +
> + return PyUnicode_FromString(dso__name(map__dso(pevent->al.map)));
> +}

Is it possible for map__dso(pevent->al.map) to return a NULL pointer here?

If a map doesn't have an associated DSO, passing it directly to dso__name(),
dso__long_name(), or dso__bid() could cause a segmentation fault. I notice
that pyrf_sample_event__srccode() safely checks if the DSO exists before
using it:

if (al.map) {
struct dso *dso = map__dso(al.map);

if (dso) {
srcfile = get_srcline_split(dso, ...);

Would it make sense to add a similar check to these getters?

> +
> +static PyObject *pyrf_sample_event__get_dso_long_name(struct pyrf_event *pevent,
> + void *closure __maybe_unused)
> +{
> + if (pyrf_sample_event__resolve_al(pevent) < 0 || !pevent->al.map)
> + Py_RETURN_NONE;
> +
> + return PyUnicode_FromString(dso__long_name(map__dso(pevent->al.map)));
> +}
> +
> +static PyObject *pyrf_sample_event__get_dso_bid(struct pyrf_event *pevent,
> + void *closure __maybe_unused)
> +{
> + char sbuild_id[SBUILD_ID_SIZE];
> +
> + if (pyrf_sample_event__resolve_al(pevent) < 0 || !pevent->al.map)
> + Py_RETURN_NONE;
> +
> + build_id__snprintf(dso__bid(map__dso(pevent->al.map)), sbuild_id, sizeof(sbuild_id));
> + return PyUnicode_FromString(sbuild_id);
> +}
> +
> +static PyObject *pyrf_sample_event__get_map_start(struct pyrf_event *pevent,
> + void *closure __maybe_unused)
> +{
> + if (pyrf_sample_event__resolve_al(pevent) < 0 || !pevent->al.map)
> + Py_RETURN_NONE;
> +
> + return PyLong_FromUnsignedLong(map__start(pevent->al.map));
> +}
> +
> +static PyObject *pyrf_sample_event__get_map_end(struct pyrf_event *pevent,
> + void *closure __maybe_unused)
> +{
> + if (pyrf_sample_event__resolve_al(pevent) < 0 || !pevent->al.map)
> + Py_RETURN_NONE;
> +
> + return PyLong_FromUnsignedLong(map__end(pevent->al.map));
> +}

Since map__start() and map__end() return 64-bit unsigned integers (u64)
to handle 64-bit virtual addresses via PAE, does passing them to
PyLong_FromUnsignedLong() truncate the addresses on 32-bit architectures?

Would it be safer to use PyLong_FromUnsignedLongLong() here, similar to
how map__pgoff() and the symbol boundaries are handled?

[ ... ]
> @@ -504,19 +838,30 @@ static PyObject *pyrf_event__new(const union perf_event *event)
> return NULL;
> }
>
> - // FIXME this better be dynamic or we need to parse everything
> - // before calling perf_mmap__consume(), including tracepoint fields.
> - if (sizeof(pevent->event) < event->header.size) {
> - PyErr_Format(PyExc_TypeError, "Unexpected event size: %zd < %u",
> - sizeof(pevent->event), event->header.size);
> - return NULL;
> - }
> -
> ptype = pyrf_event__type[event->header.type];
> - pevent = PyObject_New(struct pyrf_event, ptype);
> - if (pevent != NULL) {
> - memcpy(&pevent->event, event, event->header.size);
> - perf_sample__init(&pevent->sample, /*all=*/false);
> +
> + /* 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);
> + if (pevent == NULL)
> + return PyErr_NoMemory();
> +
> + /* Copy the event for memory safety and initilaize variables. */
> + PyObject_Init((PyObject *)pevent, ptype);

The code allocates a variable amount of memory based on event->header.size,
but the Python types (like pyrf_mmap_event__type) declare their size
statically with .tp_basicsize = sizeof(struct pyrf_event).

Is it valid in the Python C API to allocate a smaller buffer than what is
defined in .tp_basicsize?

If a malformed file provides an intentionally small header.size, could
this lead to out-of-bounds heap accesses? The PyMemberDef structures use
fixed offsets (like offsetof(struct perf_record_mmap, filename)), so if
Python scripts access these properties, they might blindly read or write
past the end of the undersized allocation.

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