Re: [PATCH v5 15/58] perf python: Add python session abstraction wrapping perf's session

sashiko-bot
2026-04-24T18:08:46Z

Sashiko AI review found 5 potential issue(s):
- [Medium] Memory leak of dynamically allocated sample data in `pyrf_session_tool__sample`.
- [Medium] Segmentation fault in `pyrf_thread__comm` when `thread__comm_str` returns NULL.
- [Medium] Python C-API violation causes SystemError when an event creation fails.
- [Medium] NULL pointer dereference in `pyrf_session__process` and `pyrf_session__process_events` due to missing initialization checks.
- [Low] Memory and reference leaks in `pyrf_session__init` if called multiple times.
--

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

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

Adds a Python wrapper for perf_session to allow processing perf.data files
directly in Python scripts. This introduces new abstractions for sessions and
threads, including a callback mechanism for handling sample events.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index a2cdd92e05482b..35eda69a32e164 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> +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);
> +}

Can thread__comm_str() return a null pointer if the thread doesn't have an
associated comm string? Passing a null pointer to PyUnicode_FromString()
would crash the Python interpreter.

[ ... ]
> +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 and returns a null pointer, it sets a Python
exception. By returning -ENOMEM here, the error bubbles up to
pyrf_session__process_events() which then returns a valid PyLong object.

Does returning a valid Python object while an exception is pending violate
the Python C-API and trigger a 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;

Does evsel__parse_sample() dynamically allocate memory for register dumps
into these fields? By explicitly setting these to null pointers, does this
overwrite the only references to that memory and cause a memory leak when
the Python object is later garbage collected?

> + if (pevent->sample.merged_callchain)
> + pevent->sample.callchain = NULL;
> +
> + ret = PyObject_CallFunction(psession->sample, "O", pyevent);
[ ... ]
> +static PyObject *pyrf_session__process(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;

If a script instantiates the session object via __new__ without calling
__init__, psession->session will be uninitialized. Could this result in a
null pointer dereference when accessing psession->session->machines?

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

Python allows __init__ to be called multiple times on the same object.
If this happens, does this leak the previously acquired references to pdata
and the old perf_session allocation?

> + perf_tool__init(&psession->tool, /*ordered_events=*/true);
> + psession->tool.ordering_requires_timestamps = true;
[ ... ]
> +static PyObject *pyrf_session__process_events(struct pyrf_session *psession)
> +{
> + int err = perf_session__process_events(psession->session);

Could psession->session be uninitialized here if the Python object was
created without calling __init__?

> + return PyLong_FromLong(err);
> +}

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