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

[Rich Felker <dalias@libc.org>]

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)
>       |                                                ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:95:16: warning: cast
> from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>    95 |     } else if (IN6_IS_ADDR_LOOPBACK(&addr6->sin6_addr) ||
>       |                ^
> /usr/include/netinet/in.h:115:24: note: expanded from macro
> 'IN6_IS_ADDR_LOOPBACK'
>   115 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:96:16: warning: cast
> from 'const struct in6_addr *' to 'unsigned char *' drops const
> qualifier [-Wcast-qual]
>    96 | IN6_IS_ADDR_LINKLOCAL(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:123:24: note: expanded from macro
> 'IN6_IS_ADDR_LINKLOCAL'
>   123 |         ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *)
> (a))[1] & 0xc0) == 0x80)
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:141:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>   141 |     } else if (IN6_IS_ADDR_V4MAPPED(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:129:24: note: expanded from macro
> 'IN6_IS_ADDR_V4MAPPED'
>   129 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:149:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>   149 |     } else if (IN6_IS_ADDR_V4COMPAT(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:134:24: note: expanded from macro
> 'IN6_IS_ADDR_V4COMPAT'
>   134 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:151:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned char *' drops const
> qualifier [-Wcast-qual]
>   151 |     } else if (IN6_IS_ADDR_SITELOCAL(&addr6->sin6_addr)) {
>       |                ^
> /usr/include/netinet/in.h:126:24: note: expanded from macro
> 'IN6_IS_ADDR_SITELOCAL'
>   126 |         ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *)
> (a))[1] & 0xc0) == 0xc0)
>       |                        ^
> /__w/c-ares/c-ares/src/lib/ares__sortaddrinfo.c:189:16: warning:
> cast from 'const struct in6_addr *' to 'unsigned int *' drops const
> qualifier [-Wcast-qual]
>   189 |     } else if (IN6_IS_ADDR_V4COMPAT(&addr6->sin6_addr) ||
>       |                ^
> /usr/include/netinet/in.h:134:24: note: expanded from macro
> 'IN6_IS_ADDR_V4COMPAT'
>   134 |         (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1]
> == 0 && \
>       |                        ^
> 
> 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_UNSPECIFIED(a) \
> -        (((uint32_t *) (a))[0] == 0 && ((uint32_t *) (a))[1] == 0 && \
> -         ((uint32_t *) (a))[2] == 0 && ((uint32_t *) (a))[3] == 0)
> +        (((const uint32_t *) (a))[0] == 0 && \
> +         ((const uint32_t *) (a))[1] == 0 && \
> +         ((const uint32_t *) (a))[2] == 0 && \
> +         ((const uint32_t *) (a))[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 )
> +        (((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 )
>  
> -#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] == 0xff)
> +#define IN6_IS_ADDR_MULTICAST(a) (((const uint8_t *) (a))[0] == 0xff)
>  
>  #define IN6_IS_ADDR_LINKLOCAL(a) \
> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0x80)
> +        ((((const uint8_t *) (a))[0]) == 0xfe && \
> +         (((const uint8_t *) (a))[1] & 0xc0) == 0x80)
>  
>  #define IN6_IS_ADDR_SITELOCAL(a) \
> -        ((((uint8_t *) (a))[0]) == 0xfe && (((uint8_t *) (a))[1] & 0xc0) == 0xc0)
> +        ((((const uint8_t *) (a))[0]) == 0xfe && \
> +         (((const uint8_t *) (a))[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)
> +        (((const uint32_t *) (a))[0] == 0 && \
> +         ((const uint32_t *) (a))[1] == 0 && \
> +         ((const uint8_t *) (a))[8] == 0 && \
> +         ((const uint8_t *) (a))[9] == 0 && \
> +         ((const uint8_t *) (a))[10] == 0xff && \
> +         ((const uint8_t *) (a))[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)
> +        (((const uint32_t *) (a))[0] == 0 && \
> +         ((const uint32_t *) (a))[1] == 0 && \
> +         ((const uint32_t *) (a))[2] == 0 && \
> +         ((const uint8_t *) (a))[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) && \
> +         ((((const uint8_t *) (a))[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) && \
> +         ((((const uint8_t *) (a))[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) && \
> +         ((((const uint8_t *) (a))[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) && \
> +         ((((const uint8_t *) (a))[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) && \
> +         ((((const uint8_t *) (a))[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]) ))

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