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

[sashiko-bot@kernel.org]

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