Re: [PATCH] trace-cmd record: Fix compression on big-endian systems

On Fri, 11 Apr 2025 19:27:56 +0200
Ilya Leoshkevich <[email protected]> wrote:

> trace-cmd report prints nothing on s390x when compression is used.
> 
> The reason is that the code treats size_t pointers as int pointers when
> serializing size_t values into 32-bit on-disk fields, which works only
> on little-endian systems.
> 
> Fix serialization by copying size_t values into int values first.
> While at it, add overflow checks.
> 
> Fixes: 176bc1f14419 ("trace-cmd record: Fix compression when files are greater than 2GB")

Thanks for the fix, but I have some comments below.

> Signed-off-by: Ilya Leoshkevich <[email protected]>
> ---
>  lib/trace-cmd/trace-compress.c | 26 +++++++++++++++++++++-----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/lib/trace-cmd/trace-compress.c b/lib/trace-cmd/trace-compress.c
> index 03215ad1..1b599043 100644
> --- a/lib/trace-cmd/trace-compress.c
> +++ b/lib/trace-cmd/trace-compress.c
> @@ -7,6 +7,7 @@
>  #include <sys/time.h>
>  #include <fcntl.h>
>  #include <errno.h>
> +#include <limits.h>
>  #include <unistd.h>
>  
>  #include "trace-cmd-private.h"
> @@ -331,7 +332,12 @@ int tracecmd_compress_block(struct tracecmd_compression *handle)
>  		goto out;
>  
>  	/* Write uncompressed data size */
> -	endian4 = tep_read_number(handle->tep, &handle->pointer, 4);
> +	if (handle->pointer > UINT_MAX) {
> +		ret = -1;
> +		goto out;
> +	}
> +	endian4 = handle->pointer;
> +	endian4 = tep_read_number(handle->tep, &endian4, 4);
>  	ret = do_write(handle, &endian4, 4);
>  	if (ret != 4) {
>  		ret = -1;
> @@ -735,13 +741,19 @@ int tracecmd_compress_copy_from(struct tracecmd_compression *handle, int fd, int
>  			}
>  			size = ret;
>  			/* Write compressed data size */
> -			endian4 = tep_read_number(handle->tep, &size, 4);
> +			if (size > UINT_MAX)
> +				break;
> +			endian4 = size;
> +			endian4 = tep_read_number(handle->tep, &endian4, 4);
>  			ret = write_fd(handle->fd, &endian4, 4);
>  			if (ret != 4)
>  				break;
>  
>  			/* Write uncompressed data size */
> -			endian4 = tep_read_number(handle->tep, &all, 4);
> +			if (all > UINT_MAX)
> +				break;
> +			endian4 = all;
> +			endian4 = tep_read_number(handle->tep, &endian4, 4);
>  			ret = write_fd(handle->fd, &endian4, 4);
>  			if (ret != 4)
>  				break;
> @@ -763,9 +775,13 @@ int tracecmd_compress_copy_from(struct tracecmd_compression *handle, int fd, int
>  	if (lseek(handle->fd, offset, SEEK_SET) == (off_t)-1)
>  		return -1;
>  
> -	endian4 = tep_read_number(handle->tep, &chunks, 4);
> +	if (chunks > UINT_MAX)
> +		return -1;
> +	endian4 = chunks;
> +	endian4 = tep_read_number(handle->tep, &endian4, 4);

You do the same conversion 4 times. Could you make a helper function
instead?

static int read_size_val(struct tep_handle *tep, size_t size, int *endian4)
{
	if (size > UINT_MAX)
		return -1;

	*endian4 = size;
	*endian4 = tep_read_number(tep, endian4, 4);
	return 0;
}

Then you can change the above to:

	if (read_size_val(handle->tep, chunks, &endian4) < 0)
		return -1;

Same with the rest.


>  	/* write chunks count*/
> -	write_fd(handle->fd, &chunks, 4);
> +	if (write_fd(handle->fd, &endian4, 4) != 4)
> +		return -1;

This is a separate issue and should be a separate patch.

>  	end_offset = lseek(handle->fd, 0, SEEK_END);
>  	if (end_offset == (off_t)-1)
>  		return -1;

Thanks,

-- Steve