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

sashiko-bot
2026-04-25T18:33:33Z

Sashiko AI review found 5 potential issue(s):
- [High] Memory leak of dynamically allocated `user_regs`, `intr_regs`, and `callchain` in `pyrf_session_tool__sample`.
- [High] Segmentation fault in `pyrf_thread__comm` when `thread__comm_str` returns `NULL`.
- [High] Python exceptions are swallowed and trigger `SystemError` crashes.
- [High] Missing initialization checks lead to `NULL` pointer dereferences in `perf.session`.
- [Medium] Repeated calls to `perf.session.__init__` leak resources.
--

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

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

This commit adds a Python wrapper around perf_session to allow using
perf.data files within a Python tool. It supports passing a sample
callback that runs during event processing and exposes thread and comm
string queries.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index a2cdd92e0548..d10359abd1ea 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> @@ -2383,6 +2389,252 @@ static int pyrf_data__setup_types(void)
> return PyType_Ready(&pyrf_data__type);
> }
>
> +struct pyrf_thread {
> + PyObject_HEAD
> +
> + struct thread *thread;
> +};

[ ... ]

> +static PyObject *pyrf_thread__comm(PyObject *obj)
> +{
> + struct pyrf_thread *pthread = (void *)obj;
> + const char *str = thread__comm_str(pthread->thread);
> +
> + return PyUnicode_FromString(str);
> +}

If the thread does not have a command string associated with it,
thread__comm_str() returns null. Does passing null to PyUnicode_FromString()
cause a segmentation fault?

Should this check the pointer and return Py_None instead?

[ ... ]

> +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() fails, it sets a Python exception and returns null. By
returning -ENOMEM here, does the error propagate up to
pyrf_session__find_thread_events() where it returns PyLong_FromLong(err)?

Returning a valid Python object while an exception is active violates the
Python C-API contract and triggers a fatal SystemError crash.

> +
> + 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;
> + if (pevent->sample.merged_callchain)
> + pevent->sample.callchain = NULL;

Since evsel__parse_sample() dynamically allocates memory for user_regs,
intr_regs, and callchain, does setting these pointers to null leak the
memory?

When the Python object is garbage collected, the cleanup function
perf_sample__exit() will be a no-op, leaking the allocations on every parsed
event with registers.

Note: This issue was fixed later in the patch series by commit "perf python:
Refactor and add accessors to sample event".

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

If the user-provided callback raises an exception, does calling PyErr_Print()
swallow the exception by clearing the Python error indicator?

This prevents the calling code from properly catching it.

[ ... ]

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

Python users can bypass __init__ by directly invoking
perf.session.__new__(perf.session). Since PyType_GenericNew
zero-initializes the object, psession->session remains null.

Will calling this method then blindly dereference psession->session and
cause a segmentation fault?

[ ... ]

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

Since Python allows calling __init__ multiple times on an existing
instance, does unconditionally overwriting psession->pdata, psession->sample,
and psession->session without releasing prior allocations leak C structures
and Python reference counts?

[ ... ]

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

Similar to the __init__ bypass mentioned above, if psession->session
is null, does passing it to perf_session__process_events() result in a
segmentation fault?

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