Re: [PATCH v3 1/2] perf record: Return the written size from process_comp_header()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] When a large buffer is split into multiple compressed records, `record__pushfn` overwrites the first record's header size with the total compressed size, corrupting the stream and causing integer truncation.
- [Medium] The `process_comp_header` function fails to initialize `event->data_size` and does not pad `PERF_RECORD_COMPRESSED2` records to an 8-byte boundary, leading to an uninitialized heap memory disclosure.
--

--- Patch [1]: [PATCH v3 1/2] perf record: Return the written size from process_comp_header() ---
commit a7fcd622c85922789abb8c299306610b9ac81440
Author: Dmitry Ilvokhin <d@ilvokhin.com>

perf record: Return the written size from process_comp_header()

This commit modifies process_comp_header() to take the remaining destination
space and return the bytes written or -1 if the header won't fit. The bounds
checking is now handled directly by the header processing function rather than
the loop in zstd_compress_stream_to_records().

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c

[ ... ]

> @@ -1591,16 +1591,25 @@ static void record__adjust_affinity(struct record *rec, struct mmap *map)
>  	}
>  }
>  
> -static size_t process_comp_header(void *record, size_t increment)
> +/*
> + * Called once with data_size == 0 to start a record, then once with
> + * data_size == compressed payload size to finalize.
> + * Returns the bytes written, or -1 if it won't fit.
> + */
> +static ssize_t process_comp_header(void *record, size_t dst_size,
> +				   size_t data_size)
>  {
>  	struct perf_record_compressed2 *event = record;
>  	size_t size = sizeof(*event);
>  
> -	if (increment) {
> -		event->header.size += increment;
> -		return increment;
> +	if (data_size) {
> +		event->header.size += data_size;
> +		return 0;
>  	}
>  
> +	if (size > dst_size)
> +		return -1;
> +
>  	event->header.type = PERF_RECORD_COMPRESSED2;
>  	event->header.size = size;

[Severity: Medium]
This is a pre-existing issue, but does process_comp_header() leave
event->data_size uninitialized here?

If called from AIO mode via record__aio_pushfn(), the map->aio.data memory
could be allocated via malloc() and reused without zeroing.

Since event->data_size isn't set and the record isn't padded to an 8-byte
boundary, this could write uninitialized heap memory to the perf.data file
and break ZSTD decompression.

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

[ ... ]

> @@ -55,12 +57,9 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  
>  	while (input.pos < input.size) {
>  		record = dst;
> -		/* process_header writes the event header into record */
> -		if (dst_size < sizeof(struct perf_event_header))
> -			goto reset;
> -		size = process_header(record, 0);
> +		size = process_header(record, dst_size, 0);
>  		/* Output buffer full — cannot fit even the record header */
> -		if (size > dst_size)
> +		if (size < 0)

[Severity: High]
This is a pre-existing issue, but when zstd_compress_stream_to_records()
splits large outputs into multiple records, doesn't returning the total
compressed size cause issues in the caller?

Looking at record__pushfn() in tools/perf/builtin-record.c, the caller seems
to assume a single record was produced and overwrites the first record's
header size:

	event->data_size = compressed - sizeof(struct perf_record_compressed2);
	event->header.size = PERF_ALIGN(compressed, sizeof(u64));
	padding = event->header.size - compressed;

Since header.size is a u16, if a chunk produces more than 65535 bytes,
this would silently truncate the value.

This would cause the padding calculation to underflow into an enormous size_t
value, and record__write() would attempt to write exabytes of zero padding to
disk.

Even without truncation, overwriting the first header would cause the
decompressor to treat subsequent intermediate headers as ZSTD payload.

>  			goto reset;
>  		compressed += size;
>  		dst += size;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783513665.git.d@ilvokhin.com?part=1