perf record: fix multi-record Zstd compression

tools/perf/util/zstd.c int zstd_fini(struct zstd_data *data) · line 36

[Severity: Critical]
This isn't a bug introduced by this patch, but is there a missing bounds check on dst_size during the first iteration in this function?

Looking at zstd_compress_stream_to_records(), the bounds check explicitly skips the first iteration because hdr_size is initialized to 0:

	while (input.pos < input.size) {
		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);
		hdr_size = size;
		compressed += size;
		dst += size;
		dst_size -= size;

If this function is called with an initially small dst_size (e.g., < 16), process_header() will perform an out-of-bounds write of the 16-byte perf_record_compressed2 header.

Following this, dst_size -= size will unconditionally unsigned underflow to a massive value. This allows ZSTD_compressStream() to write up to max_record_size bytes out-of-bounds into the dst buffer.

Could this be triggered in aio mode if perf_mmap__push() splits a wrap-around kernel ring buffer, and the first compressed chunk leaves less than 16 bytes in the aio->data buffer? The second call to zstd_compress() would then pass a dst_size < 16, bypassing the bounds check on the first iteration and triggering a buffer overflow.

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1

tools/perf/util/zstd.c ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_ · line 57

[Severity: High]
This isn't a bug introduced by this patch, but by gating the bounds check on hdr_size, isn't the check bypassed on the very first loop iteration since hdr_size is initialized to 0?

If dst_size is initially very small (which can legitimately happen in record__aio_pushfn() when calculating mmap__mmap_len(map) - aio->size), the code will unconditionally write the header and decrement dst_size.

Could this underflow dst_size to a massive positive value, bypassing subsequent bounds checks and allowing ZSTD_compressStream() to write out of bounds?

[Severity: High]
Does this check introduce an overly strict requirement that causes compression to fail prematurely?

By requiring dst_size < hdr_size + max_record_size + sizeof(u64), this condition unconditionally fails compression if the remaining buffer is less than the maximum record size (typically ~64KB).

In AIO mode, dst_size represents the remaining space in the AIO buffer, which naturally dwindles. If a small chunk needs to be compressed and could fit in the remaining space, wouldn't this check spuriously abort the compression and render the last ~64KB of every buffer unusable?

tools/perf/util/zstd.c ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_ · line 66

[Severity: High]
This isn't a bug introduced by this patch, but looking at zstd_decompress_stream() in the same file, is there an issue with how output.pos is handled?

tools/perf/util/zstd.c:zstd_decompress_stream() {
    ...
    output.dst  = dst + output.pos;
    output.size = dst_size - output.pos;
    ...
}

The loop advances the destination pointer but fails to clear output.pos.

If the decompression loop executes more than once, will ZSTD start writing at the new output.dst + output.pos, effectively skipping output.pos bytes and corrupting the decompressed stream?

[Severity: High]
This isn't a bug introduced by this patch, but does the strict bounds check introduced above make a memory leak in AIO mode more easily triggerable?

In tools/perf/builtin-record.c:record__aio_pushfn(), a reference to map->core is acquired on the first successful chunk:

tools/perf/builtin-record.c:record__aio_pushfn() {
    ...
    if (!aio->size) {
        perf_mmap__get(&map->core);
    }
    ...
}

If perf_mmap__push() processes the ring buffer in two chunks, and the second chunk's compression fails (which happens more frequently now due to the ~64KB bounds check), perf_mmap__push() aborts and passes the error back to record__aio_push():

tools/perf/builtin-record.c:record__aio_push() {
    ...
    ret = perf_mmap__push(map, &aio, record__aio_pushfn);
    if (ret != 0)
        return ret;
    ...
}

Will this return immediately on failure and bypass the perf_mmap__put() cleanup that occurs later, resulting in a leak of the map->core resource?

Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

tools/perf/util/zstd.c ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_ · line 75

[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/[email protected]?part=3