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

sashiko-bot
2026-04-23T04:25:11Z

Sashiko AI review found 3 potential issue(s):
- [High] The `fd` argument is ignored and overwritten, leading to file descriptor leaks and unintended file operations.
- [High] The `perf.data` object inadvertently closes standard input when `path='-'` is used, corrupting the Python process's file descriptor table.
- [Medium] The wrapper's file opening logic executes blocking `open()` system calls without releasing the Python Global Interpreter Lock (GIL).
--

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

perf python: Add wrapper for perf_data file abstraction

This commit introduces a Python wrapper for the perf_data file abstraction,
which is needed to provide session support.

> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 0424290f8b77f..a2cdd92e05482 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c

[ ... ]

> +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;
> +
> + if (pdata->data.open)
> + perf_data__close(&pdata->data);
> + free((char *)pdata->data.path);
> +
> + if (!path)
> + path = "perf.data";
> +
> + pdata->data.path = strdup(path);

[ ... ]

> + pdata->data.mode = PERF_DATA_MODE_READ;
> + pdata->data.file.fd = fd;
> + if (perf_data__open(&pdata->data) < 0) {

Could this introduce a regression where the provided fd is leaked?

If fd is provided but path is not, path defaults to "perf.data" earlier in
this function. When perf_data__open() is called, it relies on check_pipe()
which only recognizes a pipe if the path is exactly "-".

Will this cause perf_data__open() to assume it is a regular file, open
"perf.data" from disk, and overwrite pdata->data.file.fd instead of using the
one provided?

Additionally, is there a potential regression here regarding the Python GIL?
Since perf_data__open() can perform a blocking open() on the provided path,
could this block indefinitely without releasing the Python Global Interpreter
Lock if the path points to a FIFO or special device?

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

Does this cleanup path introduce a regression when handling standard input?

If path was set to "-", perf_data__open() sets the fd to STDIN_FILENO. When
the python object is garbage collected, does perf_data__close()
unconditionally close STDIN_FILENO and disrupt the Python process's file
descriptor table?

> + free((char *)pdata->data.path);
> + Py_TYPE(pdata)->tp_free((PyObject *)pdata);
> +}

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