[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250730132352.GK1827@brightrain.aerifal.cx>
Date: Wed, 30 Jul 2025 09:23:52 -0400
From: Rich Felker <dalias@...c.org>
To: Brad House <brad@...d-house.com>
Cc: musl@...ts.openwall.com
Subject: Re: [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros
warn on -Wcast-qual
On Fri, Jul 25, 2025 at 11:47:02AM -0400, Rich Felker wrote:
> On Thu, Nov 07, 2024 at 05:33:21AM -0500, Brad House wrote:
> > Any update on maybe merging the updated patch from this chain?
>
> Sorry, this ping slipped by while I was out of the country and not
> easily reachable. Since another bug in the current macros was just
> reported, I'll go ahead and merge it now after a quick check-over.
> Thanks again and sorry for the long wait!
The newly found bug in IN6_IS_ADDR_V4COMPAT is also preserved in this
patch, so I'm going to fix it first and rebase this on the fix. Doing
it in that order is so that the bugfix patch trivially backports.
Rich
> > On 8/14/24 5:24 PM, enh wrote:
> > > not quite the question you asked, but the new implementation is what
> > > bionic has shipped since 2016, and had the historical castful
> > > implementation before that. (citation needed?
> > > https://android-review.googlesource.com/c/platform/bionic/+/250098)
> > >
> > > On Wed, Aug 14, 2024 at 5:16 PM Rich Felker <dalias@...c.org> wrote:
> > > > On Fri, Aug 02, 2024 at 08:02:16PM -0400, Brad House wrote:
> > > > > 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
> > > > > 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
> > > > I think this looks fine. Anyone willing to point a second set of eyes
> > > > at it (or maybe write a test to check whether codegen is same before
> > > > and after) and make sure before I merge it?
> > > >
> > > > Rich
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
