From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on inbox.vuxu.org X-Spam-Level: X-Spam-Status: No, score=-3.3 required=5.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H4, RCVD_IN_MSPIKE_WL autolearn=ham autolearn_force=no version=3.4.4 Received: from second.openwall.net (second.openwall.net [193.110.157.125]) by inbox.vuxu.org (Postfix) with SMTP id 624CE2A7C7 for ; Wed, 14 Aug 2024 23:16:37 +0200 (CEST) Received: (qmail 27981 invoked by uid 550); 14 Aug 2024 21:16:32 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Reply-To: musl@lists.openwall.com Received: (qmail 27939 invoked from network); 14 Aug 2024 21:16:31 -0000 Date: Wed, 14 Aug 2024 17:16:23 -0400 From: Rich Felker To: Brad House Cc: musl@lists.openwall.com Message-ID: <20240814211623.GL10433@brightrain.aerifal.cx> References: <20240802233801.GU10433@brightrain.aerifal.cx> <4c03697d-36fd-42e3-8a27-e121d3acc522@brad-house.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4c03697d-36fd-42e3-8a27-e121d3acc522@brad-house.com> User-Agent: Mutt/1.5.21 (2010-09-15) Subject: Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual 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