[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20250725154701.GH1827@brightrain.aerifal.cx>
Date: Fri, 25 Jul 2025 11:47:02 -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 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!
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.
