Re: [PATCH 03/28] 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 6 potential issue(s) to consider:
- [High] Out-of-bounds write occurs before the newly added buffer size guard
- [High] Unsigned integer underflow and memory leak when compression yields 0 bytes
- [High] Missing context reset on buffer-full error path causes state corruption
- [High] Error propagation causes deterministic failure for incompressible data
- [High] Returning -1 on compression failure causes duplicated events
- [Medium] Ignored ZSTD_flushStream return value masks compression errors
--

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

perf zstd: Fix compression error path in zstd_compress_stream_to_records()

This commit changes the error handling in zstd_compress_stream_to_records()
from falling back to uncompressed data to returning -1 and resetting the
compression stream. It also adds a bounds check to prevent dst_size underflows.

> 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,6 +55,9 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  	while (input.pos < input.size) {
>  		record = dst;
>  		size = process_header(record, 0);

The process_header callback writes the event header to the record pointer
unconditionally. Since this happens before the newly added dst_size check,
can this cause an out-of-bounds write if the remaining dst_size is smaller
than the header size?

> +		/* Output buffer full — cannot fit even the record header */
> +		if (size > dst_size)
> +			return -1;

The patch adds a context reset for ZSTD errors so the stream is usable
for flush retries. However, this early return for a full buffer exits
without resetting the context. Will the caller retry flushing with a
partially compressed internal state, producing corrupted output?

Replacing the uncompressed fallback with returning -1 propagates the error
to perf_mmap__push.

If a ring buffer wrap occurs and the second half returns -1, will
perf_mmap__push abort and skip perf_mmap__consume, leaving the
kernel buffer tail pointer unmodified and duplicating the first half on
the next read cycle?

Additionally, if a chunk is incompressible and its size exceeds the static
destination buffer size, will the out_child retry loop overflow again and
permanently block?

[ ... ]

> @@ -65,8 +68,16 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  		ret = ZSTD_compressStream(data->cstream, &output, &input);
>  		ZSTD_flushStream(data->cstream, &output);

This isn't a bug introduced by this patch, but the return value of
ZSTD_flushStream is not captured in ret. Does the subsequent
if (ZSTD_isError(ret)) check silently discard flush errors, leading to
truncated records?

>  		if (ZSTD_isError(ret)) {

[ ... ]

>  		}
>  		size = output.pos;
>  		size = process_header(record, size);

This isn't a bug introduced by this patch, but if ZSTD_compressStream and
ZSTD_flushStream produce 0 bytes, output.pos and size will be 0.

Since process_header(record, 0) acts as an initialization command and
returns the header size again, will this code subtract the header size
from dst_size a second time?

Could this cause dst_size to underflow, completely defeating bounds
checks on the next iteration and leaking uninitialized memory into the
output?

>  		compressed += size;
>  		dst += size;
>  		dst_size -= size;
>  	}

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