Re: [PATCH v1 14/58] perf python: Add wrapper for perf_data file abstraction

sashiko-bot
2026-04-20T00:33:38Z

Sashiko AI review found 4 potential issue(s):
- [Medium] Instantiating `perf.data()` without a `path` argument causes the object destructor to attempt to `free()` a static string literal, leading to an immediate crash.
- [Medium] `pyrf_data__str` passes a potentially NULL pointer to `PyUnicode_FromString`, causing a segmentation fault.
- [Medium] The user-provided `fd` argument is silently ignored for regular files, violating the API contract.
- [Medium] Calling `__init__` multiple times on a `perf.data` object causes a memory leak and file descriptor leak.
--

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

perf python: Add wrapper for perf_data file abstraction

The perf_data struct is needed for session supported.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 46fe173c6b5cb..f0dff69f67df2 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> @@ -2279,6 +2280,73 @@ static PyObject *pyrf__metrics(PyObject *self, PyObject *args)
> return list;
> }
>
> +struct pyrf_data {
> + PyObject_HEAD
> +
> + struct perf_data data;
> +};
> +
> +static int pyrf_data__init(struct pyrf_data *pdata, PyObject *args, PyObject *kwargs)
> +{
> + static char *kwlist[] = { "path", "fd", NULL };
> + char *path = NULL;
> + int fd = -1;
> +
> + if (!PyArg_ParseTupleAndKeywords(args, kwargs, "|si", kwlist, &path, &fd))
> + return -1;
> +
> + pdata->data.path = NULL;
> + if (path) {
> + pdata->data.path = strdup(path);

What happens if the python script calls __init__ multiple times on the same
object?

If pyrf_data__init() is called a second time, it unconditionally assigns
pdata->data.path = strdup(path) without freeing the previously allocated
string. It also overwrites the previously stored file descriptor.

Since data->open might already be true, perf_data__open() will return early,
and perf_data__close() will only close the newly overwritten file descriptor
when the object is destroyed.

Will this leak both the previously allocated string memory and the original
file descriptor?

> + if (!pdata->data.path) {
> + PyErr_NoMemory();
> + return -1;
> + }
> + }
> + pdata->data.mode = PERF_DATA_MODE_READ;
> + pdata->data.file.fd = fd;
> + if (perf_data__open(&pdata->data) < 0) {

Is the user-provided fd argument ignored for regular files?

Looking at the underlying C API, perf_data__open() delegates to open_file(),
which invokes open() on the file path and unconditionally overwrites
data->file.fd with the newly generated file descriptor.

Could this cause unexpected behavior and potential file descriptor leaks if a
user passes an open file descriptor from Python?

> + PyErr_Format(PyExc_IOError, "Failed to open perf data: %s",
> + pdata->data.path ? pdata->data.path : "perf.data");
> + return -1;
> + }
> + return 0;
> +}
> +
> +static void pyrf_data__delete(struct pyrf_data *pdata)
> +{
> + perf_data__close(&pdata->data);
> + free((char *)pdata->data.path);

If path is omitted during initialization, pdata->data.path remains NULL.
Inside perf_data__open(), it checks if (!data->path) and assigns it the
static string literal "perf.data".

When the Python object is later garbage collected, this destructor is called:

pyrf_data__delete()
...
free((char *)pdata->data.path);

Can this crash the interpreter by attempting to free a static string literal
from the .rodata section?

> + Py_TYPE(pdata)->tp_free((PyObject *)pdata);
> +}
> +
> +static PyObject *pyrf_data__str(PyObject *self)
> +{
> + const struct pyrf_data *pdata = (const struct pyrf_data *)self;
> +
> + return PyUnicode_FromString(pdata->data.path);

Is it possible for pdata->data.path to be NULL here?

PyUnicode_FromString() expects a valid null-terminated string and will
segfault if passed a NULL pointer.

This pointer could remain NULL if the Python script instantiates the class
using perf.data.__new__(perf.data) without calling __init__(), or if
__init__() is called without a path but STDIN_FILENO is a pipe (causing
perf_data__open() to return early via check_pipe() before assigning the
default string).

Will calling str() or repr() on such an object cause a segmentation fault?

> +}
> +
> +static const char pyrf_data__doc[] = PyDoc_STR("perf data file object.");

[ ... ]

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