Re: [RFC PATCH v3 2/5] perf/trace-event: Write trace.dat metadata sections during parsing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Medium] Missing checks for `fseek` errors when patching section sizes can silently corrupt the output trace file.
- [Low] Missing explicit inclusion of `<stdint.h>` in `trace-dat.h`.

Pre-existing issues:
- [Critical] Integer overflow in `malloc(size + 1)` leads to a massive heap buffer overflow.
--

--- Patch [2]: [RFC PATCH v3 2/5] perf/trace-event: Write trace.dat metadata sections during parsing ---
commit 9121c22ee8da1e40687be01e1d9fb0edd88a10cd
Author: Tanushree Shah <tshah@linux.ibm.com>

perf/trace-event: Write trace.dat metadata sections during parsing

This commit modifies the perf trace event parsing logic to write trace.dat
compatible metadata sections when trace_dat_fp is set. It introduces
byte-order helpers and adds support for writing FTRACE EVENT FORMATS,
KALLSYMS, CMDLINES, and STRINGS sections.

> diff --git a/tools/perf/util/trace-dat.h b/tools/perf/util/trace-dat.h
> index 9aec37b708d41..5985083b275ac 100644
> --- a/tools/perf/util/trace-dat.h
> +++ b/tools/perf/util/trace-dat.h
> @@ -8,9 +8,13 @@
>  #define __PERF_TRACE_DAT_H
>  
>  #include <stdio.h>
> +#include <stdbool.h>
> +#include <event-parse.h>
> +#include <byteswap.h>
> +#include "util.h"

[Severity: Low]
This isn't a bug, but could the lack of an explicit <stdint.h> inclusion here
cause build failures on musl libc? The file uses uint16_t, uint32_t, and
uint64_t, and according to subsystem guidelines, all files using POSIX libc
types must explicitly include the declaring header.

[ ... ]
> --- a/tools/perf/util/trace-event-read.c
> +++ b/tools/perf/util/trace-event-read.c
[ ... ]
> @@ -338,6 +538,18 @@ static int read_event_files(struct tep_handle *pevent)
>  		}
>  		free(sys);
>  	}
> +	/* Fill in section size after writing all event files */
> +	if (trace_dat_fp && !trace_dat_write_failed) {
> +		end_pos = ftell(trace_dat_fp);
> +		section_size = to_file_u64(pevent, end_pos - sys_count_pos);
> +		fseek(trace_dat_fp, section_size_pos, SEEK_SET);
> +		if (!fwrite(&section_size, sizeof(unsigned long long), 1, trace_dat_fp)) {
> +			pr_warning("Failed to write trace.dat event formats section\n");
> +			trace_dat_write_failed = true;
> +		}
> +		fseek(trace_dat_fp, end_pos, SEEK_SET);

[Severity: Medium]
Can a failed fseek() here silently corrupt the output trace.dat file?

If the first fseek() fails due to an underlying I/O error, the subsequent
fwrite() would overwrite data at the wrong location. Since the second
fseek() is also unchecked, subsequent writes would also land at invalid
offsets.

> +	}
> +
>  	return 0;
>  }
[ ... ]
> @@ -349,8 +561,28 @@ static int read_saved_cmdline(struct tep_handle *pevent)
>  
>  	/* it can have 0 size */
>  	size = read8(pevent);
> -	if (!size)
> +	/* Write cmdlines section with empty payload if no data */
> +	if (!size) {
> +		if (trace_dat_fp && !trace_dat_write_failed) {
[ ... ]
> +		}
>  		return 0;
> +	}
>  
>  	buf = malloc(size + 1);

[Severity: Critical]
This is a pre-existing issue, but could this malloc(size + 1) trigger a heap
buffer overflow regression when parsing an untrusted perf.data file?

For example, in read_ftrace_printk(), size is a 32-bit unsigned int. If
size is UINT_MAX, size + 1 deterministically wraps to 0. This results in a
0-byte allocation, into which do_read() subsequently attempts to read
UINT_MAX bytes.

A similar issue exists here in read_saved_cmdline() on 32-bit architectures
where size_t is 32-bit.

>  	if (buf == NULL) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803145958.299956-1-tshah@linux.ibm.com?part=2