Re: [PATCH] bswap: convert to unsigned before shifting in get_be32

[Ramsay Jones <ramsay@ramsayjones.plus.com>]

On 15/07/17 20:11, René Scharfe wrote:
> The pointer p is dereferenced and we get an unsigned char.  Before
> shifting it's automatically promoted to int.  Left-shifting a signed
> 32-bit value bigger than 127 by 24 places is undefined.  Explicitly
> convert to a 32-bit unsigned type to avoid undefined behaviour if
> the highest bit is set.
> 
> Found with Clang's UBSan.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  compat/bswap.h | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/compat/bswap.h b/compat/bswap.h
> index d47c003544..4582c1107a 100644
> --- a/compat/bswap.h
> +++ b/compat/bswap.h
> @@ -166,10 +166,10 @@ static inline uint64_t git_bswap64(uint64_t x)
>  	(*((unsigned char *)(p) + 0) << 8) | \
>  	(*((unsigned char *)(p) + 1) << 0) )
>  #define get_be32(p)	( \
> -	(*((unsigned char *)(p) + 0) << 24) | \
> -	(*((unsigned char *)(p) + 1) << 16) | \
> -	(*((unsigned char *)(p) + 2) <<  8) | \
> -	(*((unsigned char *)(p) + 3) <<  0) )
> +	((uint32_t)*((unsigned char *)(p) + 0) << 24) | \
> +	((uint32_t)*((unsigned char *)(p) + 1) << 16) | \
> +	((uint32_t)*((unsigned char *)(p) + 2) <<  8) | \
> +	((uint32_t)*((unsigned char *)(p) + 3) <<  0) )
>  #define put_be32(p, v)	do { \
>  	unsigned int __v = (v); \
>  	*((unsigned char *)(p) + 0) = __v >> 24; \
> 

Heh, I have a patch that is pretty much identical. I suspect
you can guess why. ;-)

ATB,
Ramsay Jones