Re: [PATCH v4] perf data: Clean up use_stdio and structures

sashiko-bot
2026-04-08T07:57:10Z

Sashiko AI review found 2 potential issue(s):
- [Medium] Incorrect error propagation in `perf_data__open_dir` when file open fails.
- [Medium] Incomplete hardening in `perf_data__close_dir` introduces a deterministic null-pointer dereference on double-close.
--

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

perf data: Clean up use_stdio and structures

This commit reorganizes perf_data_file to better abstract the use of
stdio. It updates tools to consistently use perf_data__ accessors,
preventing implicit file descriptor usage instead of the intended fptr.

> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index 90df41da1a32b..df342a6977823 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -20,18 +20,32 @@

[ ... ]

> void perf_data__close_dir(struct perf_data *data)
> {
> close_dir(data->dir.files, data->dir.nr);
> + data->dir.files = NULL;
> }

Does this introduce a potential crash if the directory is closed twice?

By setting data->dir.files = NULL without also setting data->dir.nr = 0,
a second call to perf_data__close_dir() would pass a NULL array with a
positive count to close_dir().

Inside close_dir(), the loop while (--nr >= 0) would offset and
dereference the NULL pointer when accessing &files[nr]. Should
data->dir.nr = 0 also be set here?

[ ... ]

> @@ -132,16 +146,14 @@ int perf_data__open_dir(struct perf_data *data)
> files = file;
> file = &files[nr++];
>
> - file->path = strdup(path);
> - if (!file->path)
> + *file = (struct perf_data_file){
> + .path = strdup(path),
> + .fd = open(path, O_RDONLY),
> + .size = st.st_size,
> + .use_stdio = false,
> + };
> + if (!file->path || file->fd < 0)
> goto out_err;
> -
> - ret = open(file->path, O_RDONLY);
> - if (ret < 0)
> - goto out_err;
> -
> - file->fd = ret;
> - file->size = st.st_size;
> }

Is it intentional to return -ENOMEM when open() fails here?

Prior to this change, ret was updated with the result of open() before
checking for failure. Now, if file->fd < 0 triggers the goto out_err path,
ret still holds -ENOMEM from the realloc() step earlier in the loop. This
might provide a misleading error code to the caller.

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