Re: [PATCH 2/3] parse_pack_header_option(): avoid unaligned memory writes

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 18, 2025, 01:27 UTC
Message-ID: <xmqq7c6s52ti.fsf@gitster.g>
In-Reply-To: <20250117125530.GB2893666@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

> +	put_be32(hdr, PACK_SIGNATURE);

Tonight's comedy. PACK_SIGNATURE is defined as 0x5041434b (in pack.h) In <compat/bswap.h> we want to take advantage of the fact that assigning any unsigned integer to *(unsigned char *) would assign the integer's least significant 8 bits.

static inline void put_be32(void *ptr, uint32_t value)
{
	unsigned char *p = ptr;
	p[0] = value >> 24;
	p[1] = value >> 16;
	p[2] = value >>  8;
	p[3] = value >>  0;
}

But sparse seems not to like that.

compat/bswap.h:175:22: error: cast truncates bits from constant value (5041 becomes 41) compat/bswap.h:176:22: error: cast truncates bits from constant value (504143 becomes 43) compat/bswap.h:177:22: error: cast truncates bits from constant value (5041434b becomes 4b)

Of course we could do the mask, but should we have to?

I think the real compiler would be clever ehough to produce the identical binary with the following patch that is only needed to squelch this error, but I feel dirty after writing this.

By the way, a "git grep" finds

	put_be32(&hdr_version, INDEX_EXTENSION_VERSION2);

in the fsmonitor.c file, which does not get flagged only because the CPP macro expands to a small integer (2). That is doubly insulting.

 compat/bswap.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git c/compat/bswap.h w/compat/bswap.h
index 512f6f4b99..b34054f2bd 100644
--- c/compat/bswap.h
+++ w/compat/bswap.h
@@ -171,23 +171,23 @@ static inline uint64_t get_be64(const void *ptr)
 static inline void put_be32(void *ptr, uint32_t value)
 {
 	unsigned char *p = ptr;
-	p[0] = value >> 24;
-	p[1] = value >> 16;
-	p[2] = value >>  8;
-	p[3] = value >>  0;
+	p[0] = (value >> 24) & 0xff;
+	p[1] = (value >> 16) & 0xff;
+	p[2] = (value >>  8) & 0xff;
+	p[3] = (value >>  0) & 0xff;
 }
 
 static inline void put_be64(void *ptr, uint64_t value)
 {
 	unsigned char *p = ptr;
-	p[0] = value >> 56;
-	p[1] = value >> 48;
-	p[2] = value >> 40;
-	p[3] = value >> 32;
-	p[4] = value >> 24;
-	p[5] = value >> 16;
-	p[6] = value >>  8;
-	p[7] = value >>  0;
+	p[0] = (value >> 56) & 0xff;
+	p[1] = (value >> 48) & 0xff;
+	p[2] = (value >> 40) & 0xff;
+	p[3] = (value >> 32) & 0xff;
+	p[4] = (value >> 24) & 0xff;
+	p[5] = (value >> 16) & 0xff;
+	p[6] = (value >>  8) & 0xff;
+	p[7] = (value >>  0) & 0xff;
 }
 
 #endif /* COMPAT_BSWAP_H */