Re: [PATCH v7 14/59] perf python: Add wrapper for perf_data file abstraction

sashiko-bot
2026-04-25T23:22:44Z

Sashiko AI review found 1 potential issue(s):
- [High] The `fd` argument in `perf.data()` is silently ignored and overwritten, breaking pipe functionality and causing the wrapper to read from the wrong source.
--

commit ae4873a7e62ee38caf1c5c6d10e64f1a258b1d62
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 2ba3556c4f16..aa63c7ffe822 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
[ ... ]
> @@ -2316,6 +2317,102 @@ 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;
> +
> + if (pdata->data.open)
> + perf_data__close(&pdata->data);
> + free((char *)pdata->data.path);
> +
> + if (!path)
> + path = "perf.data";
> +
> + pdata->data.path = strdup(path);
> + if (!pdata->data.path) {
> + PyErr_NoMemory();
> + return -1;
> + }
> +
> + if (fd != -1) {
> + struct stat st;
> +
> + if (fstat(fd, &st) < 0 || !S_ISFIFO(st.st_mode)) {
> + PyErr_SetString(PyExc_ValueError,
> + "fd argument is only supported for pipes");
> + free((char *)pdata->data.path);
> + pdata->data.path = NULL;
> + return -1;
> + }
> + }
> +
> + pdata->data.mode = PERF_DATA_MODE_READ;
> + pdata->data.file.fd = fd;
> + if (perf_data__open(&pdata->data) < 0) {

Does this silently ignore the passed fd and leak it?

Looking at perf_data__open(), it delegates to check_pipe(). If path is
not explicitly provided, it defaults to "perf.data" instead of being NULL
or "-". check_pipe() requires path to be "-" to handle pipes:

tools/perf/util/data.c:check_pipe() {
...
if (!data->path) {
if (!fstat(fd, &st) && S_ISFIFO(st.st_mode))
is_pipe = true;
} else {
if (!strcmp(data->path, "-"))
is_pipe = true;
}
...
}

Since path defaults to "perf.data", check_pipe() returns false.

perf_data__open() will then call open_file(), which does a regular
filesystem open() of "perf.data", and overwrites data->file.fd:

tools/perf/util/data.c:open_file() {
...
fd = perf_data__is_read(data) ?
open_file_read(data) : open_file_write(data);
...
data->file.fd = fd;
...
}

Should the wrapper set path to "-" or NULL if a custom pipe fd is
provided so perf_data__open() correctly uses it?

> + PyErr_Format(PyExc_IOError, "Failed to open perf data: %s",
> + pdata->data.path ? pdata->data.path : "perf.data");
> + return -1;
> + }
> + return 0;
> +}

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