Re: [PATCH v7 15/59] perf python: Add python session abstraction wrapping perf's session

sashiko-bot
2026-04-25T23:29:31Z

Sashiko AI review found 5 potential issue(s):
- [High] Calling Py_XDECREF(psession->pdata) before perf_session__delete(psession->session) causes a use-after-free.
- [High] Improper Python C API exception handling causes uncatchable SystemError or silently swallowed exceptions.
- [High] Zeroing out pevent->sample.user_regs and intr_regs causes a memory leak per sample.
- [Medium] Re-initializing a perf.session object via __init__ leaks Python references and the internal perf_session C structure.
- [Medium] Calling methods on an uninitialized perf.session object causes a NULL pointer dereference.
--

commit 9762b5f02b844066c2ef3adf7767df3f2ed547e3
Author: Ian Rogers <irogers@google.com>
Date: Sat Apr 25 22:49:07 2026 +0000

perf python: Add python session abstraction wrapping perf's session

Sessions are necessary to be able to use perf.data files within a
tool. Add a wrapper python type that incorporates the tool. Allow a
sample callback to be passed when creating the session.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index aa63c7ffe822..1af53480661f 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -2413,6 +2419,256 @@ static int pyrf_data__setup_types(void)
> return PyType_Ready(&pyrf_data__type);
> }
>
[ ... ]
> +static int pyrf_session_tool__sample(const struct perf_tool *tool,
> + union perf_event *event,
> + struct perf_sample *sample,
> + struct evsel *evsel,
> + struct machine *machine __maybe_unused)
> +{
> + struct pyrf_session *psession = container_of(tool, struct pyrf_session, tool);
> + PyObject *pyevent = pyrf_event__new(event);
> + struct pyrf_event *pevent = (struct pyrf_event *)pyevent;
> + PyObject *ret;
> +
> + if (pyevent == NULL)
> + return -ENOMEM;

If pyrf_event__new() returns NULL and sets a Python exception, returning
-ENOMEM here propagates the error back to pyrf_session__find_thread_events(),
which then returns PyLong_FromLong(err).

Does returning a valid PyObject while the Python exception indicator is still
set cause the interpreter to raise an uncatchable SystemError?

> + memcpy(&pevent->event, event, event->header.size);
> + if (evsel__parse_sample(evsel, &pevent->event, &pevent->sample) < 0) {
> + Py_DECREF(pyevent);
> + return -1;
> + }
> + /* Avoid shallow copy pointing to lazily allocated memory that would be double freed. */
> + pevent->sample.user_regs = NULL;
> + pevent->sample.intr_regs = NULL;

Since evsel__parse_sample() dynamically allocates memory for user_regs and
intr_regs using zalloc(), does explicitly setting these pointers to NULL
orphan the allocations?

When the Python event object is destroyed, it looks like perf_sample__exit()
will skip freeing the NULL pointers, causing a memory leak for every
processed sample containing register data.

I see this regression is fixed later in the patch series in commit
5565efd8694262de670594ae38d7f95f563e85d1 ("perf python: Refactor and
add accessors to sample event").

> + if (pevent->sample.merged_callchain)
> + pevent->sample.callchain = NULL;
> +
> + ret = PyObject_CallFunction(psession->sample, "O", pyevent);
> + if (!ret) {
> + PyErr_Print();
> + Py_DECREF(pyevent);
> + return -1;
> + }

If the user's Python callback fails, PyErr_Print() prints the traceback
and clears the exception indicator.

Since this returns -1, does the Python caller receive an error without any
exception being raised, silently swallowing the callback failure?

> + Py_DECREF(ret);
> + Py_DECREF(pyevent);
> + return 0;
> +}
> +
> +static PyObject *pyrf_session__find_thread(struct pyrf_session *psession, PyObject *args)
> +{
> + struct machine *machine;
> + struct thread *thread = NULL;
> + PyObject *result;
> + int pid;
> +
> + if (!PyArg_ParseTuple(args, "i", &pid))
> + return NULL;
> +
> + machine = &psession->session->machines.host;

Because perf.session is created using PyType_GenericNew(), its C structure
is initially zeroed out.

If a user instantiates the object directly without calling __init__, or if
__init__ fails and leaves psession->session set to NULL, does dereferencing
it here cause a NULL pointer dereference?

> + thread = machine__find_thread(machine, pid, pid);
[ ... ]
> +static int pyrf_session__init(struct pyrf_session *psession, PyObject *args, PyObject *kwargs)
> +{
> + struct pyrf_data *pdata;
> + PyObject *sample = NULL;
> + static char *kwlist[] = { "data", "sample", NULL };
> +
> + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O!|O", kwlist, &pyrf_data__type, &pdata,
> + &sample))
> + return -1;
> +
> + Py_INCREF(pdata);
> + psession->pdata = pdata;

In Python, __init__ can be called multiple times on an already instantiated
object. If this happens, do psession->pdata, psession->sample, and
psession->session get overwritten without calling Py_XDECREF() or
perf_session__delete() on the old resources?

Could this permanently leak the old C session structure and its Python
references?

> + perf_tool__init(&psession->tool, /*ordered_events=*/true);
[ ... ]
> +static void pyrf_session__delete(struct pyrf_session *psession)
> +{
> + Py_XDECREF(psession->pdata);
> + Py_XDECREF(psession->sample);
> + perf_session__delete(psession->session);

If Py_XDECREF(psession->pdata) drops the last reference to the perf.data
object, the memory holding the embedded struct perf_data inside
struct pyrf_data is freed.

Immediately after, perf_session__delete() accesses session->data
(which points to the now-freed pdata->data) to call perf_data__close().

Could this destruction order result in a use-after-free? Should the C
session be destroyed before releasing the Python data object?

> + Py_TYPE(psession)->tp_free((PyObject *)psession);
> +}
> +
> +static PyObject *pyrf_session__find_thread_events(struct pyrf_session *psession)
> +{
> + int err = perf_session__process_events(psession->session);

Similar to pyrf_session__find_thread(), if psession->session is NULL,
will perf_session__process_events() crash?

> +
> + return PyLong_FromLong(err);
> +}

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