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

sashiko-bot
2026-04-20T01:16:57Z

Sashiko AI review found 6 potential issue(s):
- [High] Use-After-Free and state corruption in `pyrf_sample_event__srccode` when looking up arbitrary addresses.
- [Medium] Uninitialized Memory in `pyrf_event__new` causes segmentation faults and unpredictable behavior.
- [Medium] Memory and reference leak of `addr_location` objects in `pyrf_event__delete`.
- [Medium] Missing object destructors for `comm_event` and `read_event` leads to memory leaks.
- [Medium] NULL pointer dereference when accessing `.evsel` on newly created events.
- [Medium] API regression by removing `sample_time` and other common properties from non-sample events.
--

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

perf python: Refactor and add accessors to sample event

This commit adds a common field for the evsel of an event, derived from
PERF_SAMPLE_ID or PERF_SAMPLE_IDENTIFIER. It also restricts certain fields
like sample_ip to sample events and introduces new accessors for various
sample event properties.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 84a186532353e..09b5667075638 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -52,21 +60,48 @@ PyMODINIT_FUNC PyInit_perf(void);
>
> struct pyrf_event {
> PyObject_HEAD
> + /** @sample: The parsed sample from the event. */
> struct perf_sample sample;
> - union perf_event event;
> + /** @al: The address location from machine__resolve, lazily computed. */
> + struct addr_location al;
> + /** @al_resolved: True when machine__resolve been called. */
> + bool al_resolved;
> + /** @event: The underlying perf_event that may be in a file or ring buffer. */
> + union perf_event event;
> };

Could the newly added al and al_resolved fields be left containing
uninitialized memory?

If Python objects are allocated without zero-initializing the memory, these
fields might contain random data. If al_resolved evaluates to true from
garbage memory, pyrf_sample_event__resolve_al() could return immediately,
and subsequent property reads might dereference uninitialized pointers in
pevent->al.map.

Furthermore, if the parsed evsel is not attached to pevent->sample.evsel
during event reads, pyrf_event__delete() might call evsel__put() on garbage
memory, causing a crash.

> #define sample_members \
> - sample_member_def(sample_ip, ip, T_ULONGLONG, "event ip"), \
> sample_member_def(sample_pid, pid, T_INT, "event pid"), \
> sample_member_def(sample_tid, tid, T_INT, "event tid"), \
> - sample_member_def(sample_time, time, T_ULONGLONG, "event timestamp"), \
> - sample_member_def(sample_addr, addr, T_ULONGLONG, "event addr"), \
> sample_member_def(sample_id, id, T_ULONGLONG, "event id"), \
> sample_member_def(sample_stream_id, stream_id, T_ULONGLONG, "event stream id"), \
> sample_member_def(sample_period, period, T_ULONGLONG, "event period"), \
> sample_member_def(sample_cpu, cpu, T_UINT, "event cpu"),

Does removing sample_time from sample_members break existing Python scripts?
When perf_event_open is called with sample_id_all, non-sample events like
mmap_event and comm_event contain valid timestamps. Removing this property
prevents chronologically sorting streams of heterogeneous events.

> +static PyObject *pyrf_event__get_evsel(PyObject *self, void *closure __maybe_unused)
> +{
> + struct pyrf_event *pevent = (void *)self;
> +
> + return pyrf_evsel__from_evsel(pevent->sample.evsel);
> +}

What happens if this is accessed on a newly created event where
pevent->sample.evsel is NULL? It appears pyrf_evsel__from_evsel() calls
evsel__get() which immediately increments the refcount, causing a NULL
pointer dereference.

[ ... ]
> +static void pyrf_event__delete(struct pyrf_event *pevent)
> +{
> + evsel__put(pevent->sample.evsel);
> + perf_sample__exit(&pevent->sample);
> + Py_TYPE(pevent)->tp_free((PyObject *)pevent);
> +}

Does this code leak the map and thread references? If machine__resolve() was
previously called and populated pevent->al, it acquires reference counts.
It seems addr_location__exit(&pevent->al) should be called here to prevent
leaking these references upon garbage collection.

[ ... ]
> @@ -172,6 +213,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,
> };

Is the tp_dealloc assignment missing here for pyrf_comm_event__type and
pyrf_read_event__type?

The dynamic initialization was removed from pyrf_event__setup_types(), but
tp_dealloc doesn't seem to be statically assigned here. When created from C,
these objects might use the generic Python destructor, bypassing
pyrf_event__delete() and leaking evsel references and internal perf_sample
allocations.

[ ... ]
> +static PyObject *pyrf_sample_event__srccode(PyObject *self, PyObject *args)
> +{
> + struct pyrf_event *pevent = (void *)self;
> + u64 addr = pevent->sample.ip;
[ ... ]
> + if (addr != pevent->sample.ip) {
> + thread__find_symbol_fb(pevent->al.thread, pevent->sample.cpumode, addr,
> + &pevent->al);
> + }

Can this sequence cause a use-after-free of the thread object?

When thread__find_symbol_fb() is called, it passes pevent->al.thread as the
first argument while passing &pevent->al as the location output. Inside
thread__find_map() this can execute thread__zput(al->thread).

Since al aliases &pevent->al, this drops the reference to the thread. If it
was the last reference, the thread object is freed. Immediately after, it
calls thread__get(thread) using the aliased pointer which now points to
freed memory.

Additionally, does this overwrite the cached pevent->al struct, corrupting
subsequent property reads (like .symbol) for the original event IP?

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