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

[sashiko-bot@kernel.org]

Sashiko AI review found 3 potential issue(s):
- [High] Uninitialized `use_stdio` flag in `perf_data__open_dir` leads to undefined behavior and potential segmentation faults.
- [High] `perf_data__seek` triggers an assertion failure when attempting to switch output on a pipe.
- [High] Unchecked `fptr` in `perf_data_file__close` leads to `fclose(NULL)` (segmentation fault) on error paths.

--

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

    perf data: Clean up use_stdio and structures
    
    use_stdio was associated with struct perf_data and not perf_data_file
    meaning there was implicit use of fd rather than fptr that may not be
    safe. For example, in perf_data_file__write. Reorganize perf_data_file
    to better abstract use_stdio, add kernel-doc and more consistently use
    perf_data__ accessors so that use_stdio is better respected.

> diff --git a/tools/perf/util/data.c b/tools/perf/util/data.c
> index 90df41da1a32b..41993dfd6c2bc 100644
> --- a/tools/perf/util/data.c
> +++ b/tools/perf/util/data.c
> @@ -20,18 +20,30 @@
>  #include "rlimit.h"
>  #include <internal/lib.h>
>  
> -static void close_dir(struct perf_data_file *files, int nr)
> +static void perf_data_file__close(struct perf_data_file *file)
>  {
> -	while (--nr >= 0) {
> -		close(files[nr].fd);
> -		zfree(&files[nr].path);
> +	if (file->use_stdio) {
> +		fclose(file->fptr);

Does this need a check for file->fptr != NULL before calling fclose()?

If a tool initializes its output with .file.use_stdio = true and
.file.fptr = NULL (such as inject.output in cmd_inject() when using
--in-place), and an error occurs before the file is fully opened, the error
path jumps to out_close_output and calls perf_data__close(). This blindly
executes fclose(NULL), which will cause a segmentation fault.

Also, since use_stdio is read here, can it contain uninitialized memory?

In perf_data__open_dir(), the files array is dynamically grown using
realloc(files, (nr + 1) * sizeof(*files)). When a new struct perf_data_file
is appended, its path, fd, and size fields are set, but the newly added
use_stdio boolean is never explicitly initialized. Because realloc() does
not zero memory, use_stdio will contain uninitialized heap garbage.

When perf_data__close_dir() iterates over the files to close them via
this function, it reads this garbage flag. If it evaluates to true, the code
incorrectly assumes the file uses stdio and calls fclose(file->fptr). Since
fptr is part of a union with fd (or also garbage), this results in an
arbitrary pointer dereference and a segmentation fault.

[ ... ]

> +off_t perf_data__seek(struct perf_data *data, off_t offset, int whence)
> +{
> +	assert(!data->is_pipe);
> +	return perf_data_file__seek(&data->file, offset, whence);
> +}
> +
>  int perf_data__switch(struct perf_data *data,
>  		      const char *postfix,
>  		      size_t pos, bool at_exit,
> @@ -420,19 +448,18 @@ int perf_data__switch(struct perf_data *data,
>  		pr_warning("Failed to rename %s to %s\n", data->path, *new_filepath);
>  
>  	if (!at_exit) {
> -		close(data->file.fd);
> +		perf_data_file__close(&data->file);
>  		ret = perf_data__open(data);
>  		if (ret < 0)
>  			goto out;
>  
> -		if (lseek(data->file.fd, pos, SEEK_SET) == (off_t)-1) {
> +		if (perf_data__seek(data, pos, SEEK_SET) == (off_t)-1) {

Will this assertion crash the tool if a user specifies a pipe for output and
triggers an output switch (e.g., perf record -o - --switch-output)?

When the user triggers an output switch, perf_data__switch() re-opens the
pipe (correctly setting data->is_pipe = true) and then unconditionally calls
perf_data__seek() to restore the offset.

Before this patch, calling lseek() on a pipe simply returned -1 with
errno == ESPIPE and the tool handled the error gracefully. The new assertion
turns this previously handled error into a hard crash that can be reached
directly via user arguments.

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