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

sashiko-bot
2026-04-20T00:46:50Z

Sashiko AI review found 7 potential issue(s):
- [High] Double-free and double-decref in `pyrf_session__init` error paths
- [High] Memory leak and unhandled exceptions in `pyrf_session_tool__sample`
- [High] NULL pointer dereference in `pyrf_thread__comm` when thread lacks comm events
- [High] Double free of dynamically allocated `perf_sample` fields due to shallow copy
- [High] Type confusion in `pyrf_session__init` data parameter
- [Medium] Missing `.build_id` event handler prevents accurate symbol resolution
- [Medium] Unhandled C-level errors in `pyrf_session__process_events`
--

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

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

This commit adds a python session abstraction wrapping perf's session,
allowing a tool to use perf.data files. It allows passing a sample
callback during session creation, process event iteration, and retrieving
thread and comm details.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index f0dff69f67df2..b68668c267d83 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;
> + struct comm *comm = thread__comm(pthread->thread);
> + const char *str = comm__str(comm);

Can thread__comm() return NULL for threads that haven't generated comm
events? If so, will passing it to comm__str() cause a segmentation fault?
Should this check for NULL or use thread__comm_str() instead?

> +
> + return PyUnicode_FromString(str);
> +}
[ ... ]
> +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;
> +
> + if (pyevent == NULL)
> + return -ENOMEM;
> +
> + memcpy(&pevent->sample, sample, sizeof(struct perf_sample));
> + pevent->sample.evsel = evsel__get(evsel);

Does doing a shallow copy of the sample struct lead to a double free?
If the sample contains pointers to lazily allocated memory like user_regs,
when the Python wrapper is garbage collected, won't its destructor free the
pointers, leaving the perf core to double free them?

> +
> + PyObject_CallFunction(psession->sample, "O", pyevent);
> + Py_DECREF(pyevent);
> + return 0;
> +}

Does this leak memory if the callback succeeds? The return value of
PyObject_CallFunction is ignored and never decremented.

Also, if the callback raises an exception, PyObject_CallFunction returns
NULL and sets the exception indicator. Since this returns 0 and continues
the loop, will returning to the Python interpreter with an active
exception trigger a fatal SystemError?

[ ... ]
> +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, &pdata, &sample))
> + return -1;

Can the O format specifier result in type confusion? If a user passes an
integer or string instead of a perf.data object, it will be cast to
struct pyrf_data, which could lead to memory corruption when
perf_session__new() accesses it.

Should this use O! with &pyrf_data__type instead to enforce type safety?

> +
> + Py_INCREF(pdata);
> + psession->pdata = pdata;
> + perf_tool__init(&psession->tool, /*ordered_events=*/true);
> + psession->tool.ordering_requires_timestamps = true;
> +
> + #define ADD_TOOL(name) \
> + do { \
> + if (name) { \
> + if (!PyCallable_Check(name)) { \
> + PyErr_SetString(PyExc_TypeError, #name " must be callable"); \
> + return -1; \
> + } \
> + psession->tool.name = pyrf_session_tool__##name; \
> + Py_INCREF(name); \
> + psession->name = name; \
> + } \
> + } while (0)
> +
> + ADD_TOOL(sample);
> + #undef ADD_TOOL
> +
> + psession->tool.comm = perf_event__process_comm;
> + psession->tool.mmap = perf_event__process_mmap;
> + psession->tool.mmap2 = perf_event__process_mmap2;
> + psession->tool.namespaces = perf_event__process_namespaces;
> + psession->tool.cgroup = perf_event__process_cgroup;
> + psession->tool.exit = perf_event__process_exit;
> + psession->tool.fork = perf_event__process_fork;
> + psession->tool.ksymbol = perf_event__process_ksymbol;
> + psession->tool.text_poke = perf_event__process_text_poke;

Are we missing the build_id event handler here? Without registering
perf_event__process_build_id, will the machine representation lack build
IDs for DSOs, preventing the symbol subsystem from accurately resolving
addresses?

> + psession->session = perf_session__new(&pdata->data, &psession->tool);
> + if (IS_ERR(psession->session)) {
> + PyErr_Format(PyExc_IOError, "failed to create session: %ld",
> + PTR_ERR(psession->session));
> + psession->session = NULL;
> + Py_DECREF(pdata);
> + return -1;
> + }
> +
> + if (symbol__init(perf_session__env(psession->session)) < 0) {
> + perf_session__delete(psession->session);
> + Py_DECREF(pdata);
> + return -1;
> + }

If symbol__init() fails, perf_session__delete() and Py_DECREF() are
called. When tp_init returns -1, Python automatically invokes the
object's tp_dealloc method.

Since psession->session and psession->pdata aren't set to NULL after
being freed here, will the destructor unconditionally execute the cleanup
again, leading to a double-free and a fatal interpreter crash?

> +
> + if (perf_session__create_kernel_maps(psession->session) < 0)
> + pr_warning("Cannot read kernel map\n");
> +
> + return 0;
> +}
[ ... ]
> +static PyObject *pyrf_session__process_events(struct pyrf_session *psession)
> +{
> + perf_session__process_events(psession->session);
> + Py_INCREF(Py_None);
> + return Py_None;
> +}

Does ignoring the return value of perf_session__process_events() mask
C-level errors? If event processing fails due to file corruption, the
negative integer error code is swallowed, leaving Python scripts unaware
of the truncation.

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