Re: [PATCH 03/27] 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 3 potential issue(s) to consider:

Pre-existing issues:
- [High] zstd_compress_stream_to_records produces unaligned event headers when splitting streams.
- [High] record__pushfn corrupts multi-record compressed output by forcefully overwriting the first header, treating interleaved headers as payload.
- [High] zstd_decompress_stream corrupts the destination buffer and leaks memory contents due to an unreset output.pos variable.
--

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

perf zstd: Fix compression error path in zstd_compress_stream_to_records()

The error fallback does memcpy(dst, src, src_size) intending to store
uncompressed data when compression fails, but this has three bugs.

> 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

[ ... ]

> @@ -65,10 +72,18 @@ 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;
> +			goto reset;
>  		}
>  		size = output.pos;
> +		/*
> +		 * No progress: ZSTD couldn't emit any bytes into the
> +		 * remaining output buffer.  Calling process_header
> +		 * with size=0 would re-trigger header initialization,
> +		 * double-subtracting the header size from dst_size and
> +		 * underflowing the unsigned counter.
> +		 */
> +		if (size == 0)
> +			goto reset;
>  		size = process_header(record, size);
>  		compressed += size;
>  		dst += size;

[Severity: High]
This is a pre-existing issue, but does advancing dst exactly by size (which
includes output.pos, the raw compressed payload size) cause subsequent loop
iterations to write new perf_event_header structures to unaligned memory
addresses?

Since output.pos is not guaranteed to be 8-byte aligned, will this cause
unaligned memory access crashes on architectures with strict alignment
requirements, and violate the perf.data specification?

>  		dst_size -= size;
>  	}
>  
>  	return compressed;

[Severity: High]
This is a pre-existing issue, but does record__pushfn() corrupt multi-record
compressed output when calling this function?

Since zstd_compress_stream_to_records() splits large streams into multiple
independent events by interleaving raw perf_event_header structures, will
record__pushfn() forcibly overwrite the first header to span the entire
compressed byte count?

During decompression, will ZSTD_decompressStream() encounter these raw binary
headers inside the ZSTD stream and fail?

> +
> +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 is a pre-existing issue, but does zstd_decompress_stream() corrupt the
destination buffer and leak memory contents?

Inside the zstd_decompress_stream() loop, the code dynamically updates
output.dst = dst + output.pos, but does it fail to reset output.pos to 0?

Because ZSTD_decompressStream() always writes to output.dst + output.pos,
will the second iteration skip old_pos bytes of the destination buffer,
leaving them uninitialized, and write the next chunk of decompressed data
after the gap?

Note: This was fixed in commit 3c5751e39541b ("perf zstd: Fix multi-iteration
decompression and error handling") later in the series.

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