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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] `record__pushfn` overwrites the first compressed record's header with the total compressed size, corrupting multi-record streams and causing 16-bit integer overflows.
--

commit e64058a4ff7a3dc25fc5fbbc6baeab2ab6b31aa4
Author: Arnaldo Carvalho de Melo <acme@redhat.com>
Date:   Mon May 25 01:05:24 2026 +0000

    perf zstd: Fix compression error path in zstd_compress_stream_to_records()

This commit replaces the compression error fallback in
zstd_compress_stream_to_records() with an explicit error return after resetting
the ZSTD context. It also adds a new label out_child_no_flush in __cmd_record()
to skip the final flush on mmap read failures, and guards against missing
output buffer space before writing headers.

> 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;
[ ... ]
>  		size = process_header(record, size);
>  		compressed += size;
>  		dst += size;
> @@ -76,6 +91,14 @@ ssize_t zstd_compress_stream_to_records(struct zstd_data *data, void *dst, size_
>  	}
>  
>  	return compressed;

[Severity: High]
This is a pre-existing issue, but I have a question about how this returned
value is handled by the caller.

When zstd_compress_stream_to_records() emits multiple chunks and returns the
total combined size of all chunks, does the caller handle it correctly?

Looking at record__pushfn() in tools/perf/builtin-record.c, it unconditionally
assumes a single chunk is returned and overwrites the first chunk's header with
this total size:

    ssize_t compressed = zstd_compress(rec->session, map, map->data,
                                       mmap__mmap_len(map), bf, size);
    ...
    event->data_size = compressed - sizeof(struct perf_record_compressed2);
    event->header.size = PERF_ALIGN(compressed, sizeof(u64));

If the total combined size exceeds 65535 bytes, will this truncate header.size
(which is a u16) and corrupt the stream's event boundaries?

Also, by treating the concatenated chunks as a single payload, will the ZSTD
decompressor eventually read the embedded perf_event_header of the second chunk
as if it were compressed ZSTD data, irreparably corrupting the profile and
causing decompression to 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;
>  }

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