Re: [PATCH v1 20/58] perf python: Extend API for stat events in python.c

sashiko-bot
2026-04-20T00:37:29Z

Sashiko AI review found 4 potential issue(s):
- [Critical] Double free and Use-After-Free in `pyrf_session__init` error paths.
- [High] The 'stat' Python callback object is leaked when the perf session is deallocated.
- [High] Name collision in `pyrf_stat_round_event__members` silently hides the event header `type` attribute.
- [High] Python callback result leak and unhandled exceptions during event processing.
--

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

perf python: Extend API for stat events in python.c

This commit extends the perf python API by adding stat information to the
session, including callbacks and event definitions for stat events.

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index e5f96bc695fda..1e393f354ea0b 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
[ ... ]
@@ -291,6 +291,75 @@ static PyTypeObject pyrf_lost_event__type = {
[ ... ]
> +static const char pyrf_stat_round_event__doc[] = PyDoc_STR("perf stat round event object.");
> +
> +static PyMemberDef pyrf_stat_round_event__members[] = {
> + sample_members
> + member_def(perf_event_header, type, T_UINT, "event type"),
> + member_def(perf_record_stat_round, type, T_ULONGLONG, "round type"),
> + member_def(perf_record_stat_round, time, T_ULONGLONG, "round time"),
> + { .name = NULL, },
> +};

Does this name collision silently hide the event header 'type' attribute?

The array defines two members with the identical string name 'type'. When
PyType_Ready() initializes the class dictionary, the latter definition of
'type' overwrites the former.

Will accessing event.type from Python on a stat_round event yield the 64-bit
round type instead of the expected 32-bit event header type
(PERF_RECORD_STAT_ROUND)? This might break scripts that dispatch events based
on event.type.

[ ... ]
@@ -2960,6 +3074,39 @@ static int pyrf_session_tool__sample(const struct perf_tool *tool,
> return 0;
> }
>
> +static int pyrf_session_tool__stat(const struct perf_tool *tool,
> + struct perf_session *session,
> + union perf_event *event)
> +{
> + struct pyrf_session *psession = container_of(tool, struct pyrf_session, tool);
> + PyObject *pyevent = pyrf_event__new(event);
> + struct evsel *evsel = evlist__id2evsel(session->evlist, event->stat.id);
> + const char *name = evsel ? evsel__name(evsel) : "unknown";
> +
> + if (pyevent == NULL)
> + return -ENOMEM;
> +
> + PyObject_CallFunction(psession->stat, "Os", pyevent, name);
> + Py_DECREF(pyevent);
> + return 0;
> +}

Does this leak the result of the Python callback?

PyObject_CallFunction() returns a new reference to the result, which is
currently ignored and never decremented via Py_DECREF(), causing a memory
leak for every processed event.

Furthermore, if the Python callback raises an exception,
PyObject_CallFunction() returns NULL and leaves the exception set in the
Python thread state.

Should the C code check the return value to prevent returning to the Python
interpreter with a pending exception?

[ ... ]
@@ -3016,8 +3164,13 @@ static int pyrf_session__init(struct pyrf_session *psession, PyObject *args, PyO
> } while (0)
>
> ADD_TOOL(sample);
> + ADD_TOOL(stat);
> #undef ADD_TOOL

Does this leak the 'stat' callback object on session deallocation?

The ADD_TOOL macro increments the reference count of the callback argument
via Py_INCREF and stores it in psession->stat, but the destructor
pyrf_session__delete() appears to omit the corresponding
Py_XDECREF(psession->stat) call.

Also, this isn't a regression introduced by this patch, but does the error
path in pyrf_session__init() cause a double-free and use-after-free?

If symbol__init() fails, the code manually cleans up:

pyrf_session__init() {
...
if (symbol__init(perf_session__env(psession->session)) < 0) {
perf_session__delete(psession->session);
Py_DECREF(pdata);
return -1;
}
...
}

Returning -1 from a tp_init slot causes Python to tear down the partially
initialized object by invoking tp_dealloc, which maps to
pyrf_session__delete().

Because the error handlers do not clear the dangling pointers (e.g. setting
psession->session = NULL), pyrf_session__delete() will unconditionally
call Py_XDECREF(psession->pdata) and perf_session__delete(psession->session)
a second time.

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