Re: [PATCH v5 18/58] perf python: Add callchain support

sashiko-bot
2026-04-24T17:38:45Z

Sashiko AI review found 4 potential issue(s):
- [Critical] Uninitialized memory in `pyrf_event__new()` causes a crash when handling errors.
- [High] Circular reference between `pyrf_event` and `pyrf_callchain` causes a severe memory leak.
- [High] Eager callchain resolution contradicts 'lazy iteration' design, causing massive performance overhead and leaving dangerous unreachable code.
- [Medium] Unconditional execution of `calloc(0, ...)` leads to inconsistent Python API behavior across different C standard libraries.
--

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

perf python: Add callchain support

Implement pyrf_callchain_node and pyrf_callchain types for lazy
iteration over callchain frames. Add callchain property to
sample_event.

> Implement pyrf_callchain_node and pyrf_callchain types for lazy
> iteration over callchain frames. Add callchain property to

The commit message states this implements lazy iteration, but the code appears
to unconditionally and eagerly resolve the callchain for every processed sample.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 63ee9bc65721bb..28961ad47010c6 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -621,6 +624,181 @@ static PyObject *pyrf_sample_event__insn(PyObject *self, PyObject *args __maybe_
> pevent->sample.insn_len);
> }
>
[ ... ]
> +static PyObject *pyrf_callchain__next(struct pyrf_callchain *pchain)
> +{
> + struct pyrf_callchain_node *pnode;
> +
> + if (!pchain->resolved) {
> + struct evsel *evsel = pchain->pevent->sample.evsel;
> + struct evlist *evlist = evsel->evlist;
> + struct perf_session *session = evlist ? evlist__session(evlist) : NULL;
> + struct addr_location al;
> + struct callchain_cursor *cursor;
> + struct callchain_cursor_node *node;
> + u64 i;

Since pchain->resolved is hardcoded to true upon creation in
pyrf_session_tool__sample(), is this entire block unreachable dead code?

Additionally, if this were to be reached, would accessing
pchain->pevent->sample.evsel without holding an active reference to the
underlying perf_session risk a use-after-free?

[ ... ]
> @@ -848,6 +1038,7 @@ static PyObject *pyrf_event__new(const union perf_event *event)
> if (pevent != NULL) {
> memcpy(&pevent->event, event, event->header.size);
> pevent->sample.evsel = NULL;
> + pevent->callchain = NULL;
> pevent->al_resolved = false;
> addr_location__init(&pevent->al);
> }

This isn't a bug introduced by this patch, but since pevent is allocated with
PyObject_New(), wouldn't pevent->sample be left uninitialized?

If an error occurs later and Py_DECREF(pyevent) is called, pyrf_event__delete()
will call perf_sample__exit(&pevent->sample) on uninitialized garbage pointers,
which could cause a segmentation fault.

Should pevent->sample be explicitly zero-initialized during creation?

[ ... ]
> @@ -2810,6 +3001,49 @@ static int pyrf_session_tool__sample(const struct perf_tool *tool,
> if (pevent->sample.merged_callchain)
> pevent->sample.callchain = NULL;
>
> + if (sample->callchain) {
> + struct addr_location al;
> + struct callchain_cursor *cursor;
> + u64 i;
> + struct pyrf_callchain *pchain;
> +
> + addr_location__init(&al);
> + if (machine__resolve(&psession->session->machines.host, &al, sample) >= 0) {
> + cursor = get_tls_callchain_cursor();
> + if (thread__resolve_callchain(al.thread, cursor, evsel, sample,
> + NULL, NULL, PERF_MAX_STACK_DEPTH) == 0) {
> + callchain_cursor_commit(cursor);
> +
> + pchain = PyObject_New(struct pyrf_callchain, &pyrf_callchain__type);
> + if (pchain) {
> + pchain->pevent = pevent;
> + Py_INCREF(pevent);

Doesn't this create a circular reference? The pchain object stores a strong
reference to the event here, and below, the event stores a reference to the
callchain.

Since neither pyrf_event__type nor pyrf_callchain__type implements the Python
cyclic garbage collector protocols, will this permanently leak both the event
and the callchain objects (along with all mapped C structures) on every
processed sample?

> + pchain->nr_frames = cursor->nr;
> + pchain->pos = 0;
> + pchain->resolved = true;
> + pchain->frames = calloc(pchain->nr_frames,
> + sizeof(*pchain->frames));

If cursor->nr is 0, this calls calloc(0, ...). Since the behavior of calloc(0)
is implementation-defined, couldn't this return NULL on some systems (treating
it as an allocation failure) and a unique zero-sized pointer on others?

Will this cause the exact same perf.data file to unpredictably yield either
None or an empty iterable depending on the host's C library?

> + if (pchain->frames) {
> + struct callchain_cursor_node *node;
> +
> + for (i = 0; i < pchain->nr_frames; i++) {
> + node = callchain_cursor_current(cursor);
> + pchain->frames[i].ip = node->ip;
> + pchain->frames[i].map =
> + map__get(node->ms.map);
> + pchain->frames[i].sym = node->ms.sym;
> + callchain_cursor_advance(cursor);
> + }
> + pevent->callchain = (PyObject *)pchain;

The event stores the reference to the callchain here, completing the circular
reference.

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