Re: [PATCH 18/29] perf header: Propagate feature section processing errors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Skipping tracepoint event preparation for truncated files causes tools to parse trace data using the local system's tracefs format instead of the recorded format.
--

commit 6b472d4fefea1e2e6c372f8fb473b94680a6af08
Author: Arnaldo Carvalho de Melo <acme@redhat.com>

perf header: Propagate feature section processing errors

This commit updates perf_session__read_header() to propagate errors from
feature section processors, ensuring invalid perf.data files are rejected
rather than partially initialized. It also modifies the behavior for
truncated files to skip feature section processing and clear the feature
bitmap.

> diff --git a/tools/perf/util/header.c b/tools/perf/util/header.c
> --- a/tools/perf/util/header.c
> +++ b/tools/perf/util/header.c

[ ... ]

> @@ -4997,15 +5001,36 @@ int perf_session__read_header(struct perf_session *session)
>  		lseek(fd, tmp, SEEK_SET);
>  	}
>  
> +	/*
> +	 * Skip feature section processing for truncated files
> +	 * (data.size == 0 means recording was interrupted).  The
> +	 * section table is unreliable in that case, and the event
> +	 * data can still be processed without the feature headers.
> +	 * Clear the bitmap so has_feat() returns false and tools
> +	 * use their "feature not present" fallbacks instead of
> +	 * accessing uninitialized env fields.
> +	 */
> +	if (f_header.data.size == 0) {
> +		bitmap_zero(header->adds_features, HEADER_FEAT_BITS);
> +	} else {
>  #ifdef HAVE_LIBTRACEEVENT
> -	perf_header__process_sections(header, fd, &session->tevent,
> -				      perf_file_section__process);
> +		err = perf_header__process_sections(header, fd, &session->tevent,
> +						    perf_file_section__process);
> +		if (err < 0)
> +			goto out_delete_evlist;
>  
> -	if (evlist__prepare_tracepoint_events(session->evlist, session->tevent.pevent))
> -		goto out_delete_evlist;
> +		if (evlist__prepare_tracepoint_events(session->evlist,
> +						      session->tevent.pevent)) {
> +			err = -ENOMEM;
> +			goto out_delete_evlist;
> +		}
>  #else

[Severity: Medium]
When f_header.data.size == 0, the new logic skips calling
evlist__prepare_tracepoint_events(). Does this cause tools to parse trace
data using the local system's tracefs format instead of the recorded format?

If evlist__prepare_tracepoint_events() is bypassed, any tracepoint events
present in the file will lack their tp_format initialization. Later, when
processing these samples, evsel__tp_format() will find tp_format == NULL
and fall back to reading the format from the local host's
/sys/kernel/tracing/events.

This could lead to out-of-bounds reads or garbage data extraction if the
binary payload from the perf.data file is interpreted using a local tracepoint
format that belongs to a different kernel version or architecture. Before this
change, evlist__prepare_tracepoint_events() was called unconditionally.

> -	perf_header__process_sections(header, fd, NULL, perf_file_section__process);
> +		err = perf_header__process_sections(header, fd, NULL,
> +						    perf_file_section__process);
> +		if (err < 0)
> +			goto out_delete_evlist;
>  #endif
> +	}
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525010550.1100375-1-acme@kernel.org?part=18