Re: [PATCH] bswap: convert to unsigned before shifting in get_be32
- From
Ramsay Jones <ramsay@ramsayjones.plus.com>
- Date
- Jul 16, 2017, 00:23 UTC
- Message-ID
- <5fd84b40-9dde-0fd2-694c-49a073e87495@ramsayjones.plus.com>
- In-Reply-To
- <56f1263c-a0b4-260b-7e23-881e21119041@web.de>
On 15/07/17 20:11, René Scharfe wrote:
Show 33 quoted lines
> 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