Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual

[Brad House <brad@brad-house.com>]

[-- Attachment #1: Type: text/plain, Size: 2814 bytes --]

On 8/2/24 7:38 PM, Rich Felker wrote:

> On Fri, Aug 02, 2024 at 05:27:26PM -0400, Brad House wrote:
>> I'm the maintainer of c-ares (https://c-ares.org) and have been
>> scanning the CI build logs for various systems to catch warnings,
>> and on Alpine Linux (which obviously uses musl c) we get these
>> warnings, specifically when using clang (but not oddly not on gcc):
>>
>> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:93:9: warning: cast
>> from 'const struct in6_addr *' to 'unsigned char *' drops const
>> qualifier [-Wcast-qual]
>>     93 |     if (IN6_IS_ADDR_MULTICAST(&addr6->sin6_addr)) {
>>        |         ^
>> /usr/include/netinet/in.h:120:48: note: expanded from macro
>> 'IN6_IS_ADDR_MULTICAST'
>>    120 | #define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
>>        |                                                ^
>> ...       ^
>>
>> Full build output: https://github.com/c-ares/c-ares/actions/runs/10219723015/job/28278549865
>>
>> I've attached a patch that will silence this warning by always
>> casting to the comparison to const, but otherwise not impact the
>> behavior.
>>
>> -Brad
>> diff --git a/include/netinet/in.h b/include/netinet/in.h
>> index fb628b61..f04657f3 100644
>> --- a/include/netinet/in.h
>> +++ b/include/netinet/in.h
>> @@ -108,46 +108,63 @@ uint16_t ntohs(uint16_t);
>>   #define IPPROTO_MAX      263
>>   
>> ...
>>   #define IN6_IS_ADDR_LOOPBACK(a) \
>> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
>> -         ((uint32_t *) (a))[2] == 0 && \
>> -         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
>> -         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
>> +        (((const uint32_t *) (a))[0] == 0 && \
>> +         ((const uint32_t *) (a))[1] == 0 && \
>> +         ((const uint32_t *) (a))[2] == 0 && \
>> +         ((const uint8_t *) (a))[12] == 0 && \
>> +         ((const uint8_t *) (a))[13] == 0 && \
>> +         ((const uint8_t *) (a))[14] == 0 && \
>> +         ((const uint8_t *) (a))[15] == 1 )
>> ...
> It looks like there's a lot wrong with these macros. They should not
> be doing random pointer casts like they are. Per the standard, they
> take an argument of type const struct in6_addr *, so they should
> almost surely be operating on that type directly. That would make them
> actually type-safe (diagnostic if called with wrong argument type).
>
> I guess we should look at whether there's any good reason they were
> written the way they were..
>
> Rich

Yep, I see what you mean.  There are already accessors for 8, 16, and 
32bit into struct in6_addr so its odd not to use those.  I've attached a 
v2 patch that uses those instead which also cleans up the warnings.

-Brad

[-- Attachment #2: musl-netinet-warning-fix-v2.patch --]
[-- Type: text/plain, Size: 3845 bytes --]

diff --git a/include/netinet/in.h b/include/netinet/in.h
index fb628b61..c6afeed8 100644
--- a/include/netinet/in.h
+++ b/include/netinet/in.h
@@ -108,51 +108,68 @@ uint16_t ntohs(uint16_t);
 #define IPPROTO_MAX      263
 
 #define IN6_IS_ADDR_UNSPECIFIED(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr32)[2] == 0 && \
+         ((a)->s6_addr32)[3] == 0)
 
 #define IN6_IS_ADDR_LOOPBACK(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && \
-         ((uint8_t *) (a))[12] == 0 && ((uint8_t *) (a))[13] == 0 && \
-         ((uint8_t *) (a))[14] == 0 && ((uint8_t *) (a))[15] == 1 )
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr32)[2] == 0 && \
+         ((a)->s6_addr)[12] == 0 && \
+         ((a)->s6_addr)[13] == 0 && \
+         ((a)->s6_addr)[14] == 0 && \
+         ((a)->s6_addr)[15] == 1 )
 
-#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
+#define IN6_IS_ADDR_MULTICAST(a) (((a)->s6_addr)[0] == 0xff)
 
 #define IN6_IS_ADDR_LINKLOCAL(a) \
-        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
+        ((((a)->s6_addr)[0]) == 0xfe && \
+         (((a)->s6_addr)[1] & 0xc0) == 0x80)
 
 #define IN6_IS_ADDR_SITELOCAL(a) \
-        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
+        ((((a)->s6_addr)[0]) == 0xfe && \
+         (((a)->s6_addr)[1] & 0xc0) == 0xc0)
 
 #define IN6_IS_ADDR_V4MAPPED(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint8_t *) (a))[8] == 0 && ((uint8_t *) (a))[9] == 0 && \
-         ((uint8_t *) (a))[10] == 0xff && ((uint8_t *) (a))[11] == 0xff)
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr)[8] == 0 && \
+         ((a)->s6_addr)[9] == 0 && \
+         ((a)->s6_addr)[10] == 0xff && \
+         ((a)->s6_addr)[11] == 0xff)
 
 #define IN6_IS_ADDR_V4COMPAT(a) \
-        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
-         ((uint32_t *) (a))[2] == 0 && ((uint8_t *) (a))[15] > 1)
+        (((a)->s6_addr32)[0] == 0 && \
+         ((a)->s6_addr32)[1] == 0 && \
+         ((a)->s6_addr32)[2] == 0 && \
+         ((a)->s6_addr)[15] > 1)
 
 #define IN6_IS_ADDR_MC_NODELOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x1))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x1))
 
 #define IN6_IS_ADDR_MC_LINKLOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x2))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x2))
 
 #define IN6_IS_ADDR_MC_SITELOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x5))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x5))
 
 #define IN6_IS_ADDR_MC_ORGLOCAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0x8))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0x8))
 
 #define IN6_IS_ADDR_MC_GLOBAL(a) \
-        (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) == 0xe))
+        (IN6_IS_ADDR_MULTICAST(a) && \
+         ((((a)->s6_addr)[1] & 0xf) == 0xe))
 
 #define __ARE_4_EQUAL(a,b) \
 	(!( (0[a]-0[b]) | (1[a]-1[b]) | (2[a]-2[b]) | (3[a]-3[b]) ))
 #define IN6_ARE_ADDR_EQUAL(a,b) \
-	__ARE_4_EQUAL((const uint32_t *)(a), (const uint32_t *)(b))
+	__ARE_4_EQUAL((a)->s6_addr32, (b)->s6_addr32)
 
 #define	IN_CLASSA(a)		((((in_addr_t)(a)) & 0x80000000) == 0)
 #define	IN_CLASSA_NET		0xff000000