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

sashiko-bot
2026-04-23T05:29:55Z

Sashiko AI review found 4 potential issue(s):
- [Critical] Removing perf_sample__init from pyrf_event__new leaves pevent->sample pointer fields uninitialized, leading to a wild free crash during event deallocation.
- [High] The Python property getters pyrf_sample_event__get_dso, pyrf_sample_event__get_dso_long_name, and pyrf_sample_event__get_dso_bid dereference the DSO pointer without checking if it is NULL.
- [High] A failure in evsel__parse_sample causes a double evsel__put() on the event's evsel reference, leading to a refcount underflow.
- [Medium] pyrf_comm_event__type and pyrf_read_event__type are missing the .tp_new and .tp_dealloc definitions.
--

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

perf python: Refactor and add accessors to sample event

This commit adds a common evsel field for events and moves sample-specific
fields to only be present in sample events. It also introduces new accessors
for sample events and refactors object type initialization in the Python
bindings.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index d90aa24e53105..3e0534744f0a2 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 pyrf_comm_event__type and pyrf_read_event__type missing their .tp_new and
.tp_dealloc assignments after the initialization refactor?

Without these, it appears direct Python instantiation could break, and missing
the C-level destructors could leave their lifecycle management inconsistent
compared to other perf event objects.

[ ... ]

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

Could map__dso(pevent->al.map) return NULL here?

The accessors check if pevent->al.map is NULL, but if map__dso() itself returns
NULL, passing it to dso__name(), dso__long_name(), or dso__bid() could trigger
a NULL pointer dereference.

[ ... ]

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

Since PyObject_New() does not zero-initialize memory, does removing
perf_sample__init() leave pointer fields in pevent->sample uninitialized?

If pevent is later garbage collected (for non-sample events, or if
evlist__event2evsel() fails below), pyrf_event__delete() will call
perf_sample__exit(&pevent->sample). This attempts to free pointers like
sample->user_regs or sample->intr_regs. Could this lead to a wild free crash
when it operates on uninitialized heap memory?

[ ... ]

> @@ -1807,8 +2137,7 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
> return pyevent;
> }
> end:
> - Py_INCREF(Py_None);
> - return Py_None;
> + Py_RETURN_NONE;
> }

While not introduced directly by this patch, it seems the failure path for
evsel__parse_sample() right above this block might have a refcount underflow
issue:

err = evsel__parse_sample(evsel, &pevent->event, &pevent->sample);
if (err) {
Py_DECREF(pyevent);

If evsel__parse_sample() fails, it cleans up by calling perf_sample__exit(),
which calls evsel__put() on the evsel reference without setting sample->evsel
to NULL.

When Py_DECREF(pyevent) is subsequently called here, it triggers the
pyrf_event__delete() destructor, which will call perf_sample__exit() a second
time. Does this cause a double evsel__put() and potential use-after-free?

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