Re: [PATCH 04/29] perf zstd: Fix compression error path in zstd_compress_stream_to_records()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Mishandling of multi-record compressed streams leads to header size overflow, stream corruption, and out-of-bounds stack reads.
- [High] AIO compressed records contain an uninitialized `data_size` field, breaking decompression.
- [High] `ZSTD_flushStream()` is invoked on potentially errored contexts and its return value is ignored, causing silent data loss.
- [High] `zstd_decompress_stream` manually advances the output buffer pointers without resetting `output.pos`, leading to double-increment and uninitialized memory gaps.
--

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

perf zstd: Fix compression error path in zstd_compress_stream_to_records()

This commit fixes multiple issues in the error paths of
zstd_compress_stream_to_records(). It properly resets the ZSTD
compression context on failures instead of incorrectly copying
uncompressed data, and ensures process_header() doesn't write past the
output buffer. It also fixes dst_size underflows and adds an
out_child_no_flush label to prevent retrying failed mmap reads.

> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index cc601796b2c8a..f1877bac815d7 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
[ ... ]
> diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> index 57027e0ac7b65..ecda9deb53b73 100644
> --- a/tools/perf/util/zstd.c
> +++ b/tools/perf/util/zstd.c
[ ... ]
> @@ -54,7 +55,13 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  
>  	while (input.pos < input.size) {
>  		record = dst;

[Severity: Critical]
This is a pre-existing issue, but does returning the aggregate size of
multiple compressed records from this loop cause problems for callers like
record__pushfn()?

If record__pushfn() assumes the output is a single record and assigns
the total aggregate size to event->header.size, a total compressed size
exceeding 65535 bytes could cause the 16-bit header.size to overflow and
truncate.

This truncation could cause an underflow in the subsequent padding
calculation (padding = event->header.size - compressed), resulting in a
massive size passed to record__write(&pad, padding) which would dump
out-of-bounds stack memory into the trace file.

> +		/* process_header writes the event header into record */
> +		if (dst_size < sizeof(struct perf_event_header))
> +			goto reset;
>  		size = process_header(record, 0);

[Severity: High]
This isn't a bug introduced by this patch, but does the process_header
callback leave the 8-byte data_size field uninitialized in AIO mode?

In AIO mode, record__aio_pushfn() calls zstd_compress() but does not
manually set event->data_size. When process_header() initializes
header.type and header.size, the data_size field appears to be left
uninitialized.

This causes perf_session__process_compressed_event() to fail
decompression when reading perf.data.

> +		/* Output buffer full — cannot fit even the record header */
> +		if (size > dst_size)
> +			goto reset;
>  		compressed += size;
>  		dst += size;
>  		dst_size -= size;
[ ... ]
> @@ -65,10 +72,18 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  		if (ZSTD_isError(ret)) {

[Severity: High]
This is a pre-existing issue, but does calling ZSTD_flushStream() just
before this error check, without verifying if ZSTD_compressStream()
succeeded, violate the API contract?

Also, the return value of ZSTD_flushStream() is discarded. If the output
buffer fills up during flush, ZSTD returns > 0 to indicate more flushing
is required.

Because the loop only evaluates input.pos < input.size, if the input is
exhausted but the flush is incomplete, the loop will exit and leave the
remaining compressed data trapped in the internal context, dropping data.

>  			pr_err("failed to compress %ld bytes: %s\n",
>  				(long)src_size, ZSTD_getErrorName(ret));
> -			memcpy(dst, src, src_size);
> -			return src_size;
> +			goto reset;
>  		}
>  		size = output.pos;
[ ... ]
> @@ -76,6 +91,14 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  	}
>  
>  	return compressed;
> +
> +reset:
> +	/* Reset so the context is usable if the caller retries */
> +	ret = ZSTD_initCStream(data->cstream, data->comp_level);
> +	if (ZSTD_isError(ret))
> +		pr_err("failed to reset compression context: %s\n",
> +			ZSTD_getErrorName(ret));
> +	return -1;
>  }
>  
>  size_t zstd_decompress_stream(struct zstd_data *data, void *src, size_t src_size,

[Severity: High]
This isn't a bug introduced by this patch, but does
zstd_decompress_stream() fail to reset output.pos to 0 during each
iteration?

At the end of the decompression loop:

    output.dst  = dst + output.pos;
    output.size = dst_size - output.pos;

Without resetting output.pos = 0, the next iteration will write at
(dst + pos) + pos, skipping memory and leaving uninitialized gaps.

I see this was fixed later in the series in commit e38284f65bb96
("perf zstd: Fix multi-iteration decompression and error handling").

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