Re: [PATCH 3/3] perf record: Don't store raw data on zstd compression failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] The `zstd_decompress_stream` function incorrectly updates the output buffer pointer, causing an out-of-bounds write and data corruption.
- [Critical] Buffer overflow via integer underflow in `zstd_compress_stream_to_records` bypasses the bounds check.
- [High] Unflushed compressed data is left in the ZSTD internal buffer and permanently lost at the end of a recording session.
- [High] The return value of `ZSTD_flushStream()` is entirely ignored, masking potential compression state errors.
- [High] A missing `perf_mmap__put` in the error path of `record__aio_push` causes a reference count leak of `map->core`.
- [High] Skipped `perf_mmap__consume` on partial failure causes duplicate event records in the `perf.data` file.
- [High] An overly strict bounds check in `zstd_compress_stream_to_records` causes `perf record` to abort when compressing large chunks of trace data.
--

commit 460b94129652aafbc57f9667aa7c4a4a81ffacbe
Author: Dmitry Ilvokhin <d@ilvokhin.com>

perf record: Don't store raw data on zstd compression failure

On a ZSTD_compressStream() error, zstd_compress_stream_to_records()
falls back to copying the uncompressed input into the output buffer and
returning its size. That cannot work:

- a COMPRESSED2 payload is always fed to zstd_decompress_stream(), so
  raw bytes placed there can never be read back.

- the record header is never finalized: process_header() is not called
  to write the real header.size and data_size, so they keep their
  initial values and do not match the bytes written.

- the copy is unbounded and can write past the output buffer.

Propagate the error instead.

Fixes: f24c1d7523e6 ("perf tools: Introduce Zstd streaming based compression API")

> diff --git a/tools/perf/util/zstd.c b/tools/perf/util/zstd.c
> index a0f6572ad2017..2de777d49ee0b 100644
> --- a/tools/perf/util/zstd.c
> +++ b/tools/perf/util/zstd.c
[ ... ]
> @@ -71,8 +69,7 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  		if (ZSTD_isError(ret)) {
>  			pr_err("failed to compress %ld bytes: %s\n",
>  				(long)src_size, ZSTD_getErrorName(ret));
> -			memcpy(dst, src, src_size);
> -			return src_size;
> +			return -1;
>  		}
>  		size = output.pos;
>  		size = process_header(record, size);

[Severity: High]
This isn't a bug introduced by this patch, but does the loop ignore the
return value of ZSTD_flushStream()?

Looking at zstd_compress_stream_to_records():

		ret = ZSTD_compressStream(data->cstream, &output, &input);
		ZSTD_flushStream(data->cstream, &output);
		if (ZSTD_isError(ret)) {

The return value of ZSTD_flushStream() is never checked. The subsequent
ZSTD_isError(ret) only evaluates the error code of ZSTD_compressStream().
If the flush operation encounters a failure, the error code is discarded,
potentially resulting in silently corrupted data streams or emitting an
incomplete compressed payload without propagating an error to the user.


[Severity: High]
This is a pre-existing issue, but does zstd_compress_stream_to_records()
leave unflushed compressed data in the ZSTD internal buffer?

In tools/perf/util/zstd.c:zstd_compress_stream_to_records():

	while (input.pos < input.size) {
        ...

The loop exits strictly when input.pos == input.size. Since
ZSTD_flushStream() can leave unflushed data in the internal compression
stream if the output buffer fills up, and perf record never calls
ZSTD_endStream() to force a final flush at the end of the session, any
remaining compressed data trapped in the stream buffer is silently and
permanently lost, resulting in truncated or corrupted trace files.


[Severity: Critical]
This is a pre-existing issue, but does zstd_decompress_stream() incorrectly
advance the output buffer pointer, causing an out-of-bounds write?

In tools/perf/util/zstd.c:zstd_decompress_stream():

	while (input.pos < input.size) {
		ret = ZSTD_decompressStream(data->dstream, &output, &input);
        ...
		output.dst  = dst + output.pos;
		output.size = dst_size - output.pos;
	}

The output.dst pointer is advanced by output.pos
(output.dst = dst + output.pos). However, output.pos is the accumulated
number of bytes written, which ZSTD_decompressStream() internally adds to
output.dst. This evaluates to an effective write address of
dst + 2 * output.pos.

Consequently, the decompression loop skips over output.pos bytes on every
iteration, leaving uninitialized gaps in the decompressed output and quickly
writing past the dst_size bounds, causing severe memory corruption.


[Severity: Critical]
This is a pre-existing issue, but does zstd_compress_stream_to_records()
have a buffer overflow via integer underflow that bypasses bounds checking?

In tools/perf/util/zstd.c:zstd_compress_stream_to_records():

		if (hdr_size &&
		    dst_size < hdr_size + max_record_size + sizeof(u64)) {
			pr_err("failed to compress: output buffer too small\n");
			return -1;
		}
		record = dst;
		size = process_header(record, 0);

When perf_mmap__push() splits the ring buffer, the second chunk can be
passed to zstd_compress() with a very small dst_size (e.g., 8 bytes). Since
hdr_size is initially 0, the boundary check is bypassed on the first
iteration.

process_header() unconditionally writes a 24-byte header into the <24 byte
remaining buffer, corrupting memory. Then dst_size -= 24 underflows to a
huge value (SIZE_MAX), causing ZSTD_compressStream() to write up to 64KB
(max_record_size) entirely out of bounds.


[Severity: High]
This is a pre-existing issue, but does the error path in record__aio_push()
cause a reference count leak of map->core?

In tools/perf/builtin-record.c:record__aio_push():

	if (ret != 0) /* ret > 0 - no data, ret < 0 - error */
		return ret;

When zstd_compress() fails on the second mmap chunk, record__aio_pushfn()
propagates the error (< 0). perf_mmap__push() then aborts and returns the
error code.

The code checks if (ret != 0) and returns immediately, skipping the
perf_mmap__put(&map->core) cleanup that balances the perf_mmap__get()
acquired during the successful first chunk. This unhandled error path causes
an LSAN-reportable memory leak and violates perf tool reference counting
invariants.


[Severity: High]
This is a pre-existing issue, but does skipping perf_mmap__consume() on
partial failure cause duplicate event records in the perf.data file?

In tools/perf/util/mmap.c:perf_mmap__push():

	if (push(md, to, buf, size) < 0) {
		rc = -1;
		goto out;
	}

If the second chunk of a split ring buffer fails during zstd_compress(),
perf_mmap__push() aborts via goto out, skipping the
perf_mmap__consume(&md->core) call.

Because perf_mmap__consume() is skipped, the ring buffer's consumer tail
pointer is never advanced, even though the first chunk was successfully
compressed and written. The caller record__mmap_read_evlist() receives the
error, aborts, and triggers a retry via record__mmap_read_all(rec, true),
which reads and writes the exact same first chunk again, resulting in
duplicated events and corrupted profiling data.


[Severity: High]
This is a pre-existing issue, but does an overly strict bounds check in
zstd_compress_stream_to_records() cause perf record to improperly abort?

In tools/perf/util/zstd.c:zstd_compress_stream_to_records():

		if (hdr_size &&
		    dst_size < hdr_size + max_record_size + sizeof(u64)) {
			pr_err("failed to compress: output buffer too small\n");
			return -1;
		}

The loop requires dst_size to be larger than approximately 64KB
(hdr_size + max_record_size + sizeof(u64)) to proceed to the next iteration.

If the remaining buffer space falls below this threshold near the end of a
compression loop, it unconditionally returns -1 and prints "output buffer
too small", even if only a few bytes of input remain to be compressed. This
logic effectively turns a normal buffer sizing condition into a fatal error,
instantly aborting the entire perf record session.

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