From: Ramsay Jones Date: Sun, 16 Jul 2017 00:23:57 GMT Subject: Re: [PATCH] bswap: convert to unsigned before shifting in get_be32 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: > 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 > --- > 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