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.1 required=5.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,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 7F64424418 for ; Wed, 14 Aug 2024 23:25:12 +0200 (CEST) Received: (qmail 26143 invoked by uid 550); 14 Aug 2024 21:25:07 -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 26090 invoked from network); 14 Aug 2024 21:25:07 -0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20230601; t=1723670699; x=1724275499; darn=lists.openwall.com; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=jXrjhMv9N1VQirXdQYglnq/v/NGMU0fFfTNP7l4cwWU=; b=aUIQQyAPsxpyM61MKnGdInIObVLj8tjcOVwJssdUmk8+a87dKxQsmBnLSDffkn9wzf wYl3WTdNhgSeSah4SYLlTIWHbC8g/LTXESL2zhJKFqkJt76Nmyogj7SR6a5D+4+iaHZf pLqDjW0uFEUV1EDtUPIJmgyXi1ayy8ClJunt1/sXIAR4cV2tUmkvwd3cmUyymPnMgGln ZyzdPYM5wnmGdy8Zz4SKDIzzsX2tjaK5OFrB3BkZ42yZ8avCUx70+D8i/pUEZNjX16HQ inA+kXY4PuiAxcp4tU41mi6qMyyQbn/cVg4+4WFkAAldrKxDDzXkIEvgdlYQPhZgDI5R oZBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1723670699; x=1724275499; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=jXrjhMv9N1VQirXdQYglnq/v/NGMU0fFfTNP7l4cwWU=; b=hNCkFlys7tt4KaKLtR1z/jk/SQVibiPkd8XJHqni6I/xYBWg/p3fTEVLSkfjVMva4n LMVou6xo8erPGTM9zce6w/NAPFKZFpCLh5E1YxQcIuOSlZ+MMcCjcUHeBirwXyEhlsGF HaqlxsJMsebf8I9hicANmcaHvCNawMOuD0G07gqKGVhXDjuO2eTzFKUQ1qjB/KUAvPus BIJZnDaPR+sn1l8AyxdyG9o2J829cB3M17RpfP3Pu+tSbKCqDMUbSLp0ceINluMwdtHa eK840vCwq+ICRrXYa5QXR3Q+GGDmJhS3QxuG1epQcWaztj93ufHNTZ4EI7RvT3LToi5g pyBA== X-Gm-Message-State: AOJu0YzRdcjAoGHV6t9CV0kZdsLEQ5BJOpO3+FrcMhkEKPOJMpcPeAsl j4wvfKCMf1JQ4x9dKkKn9VE0+E2/4yhMl8dF2mgTz4qAT3sXLVbNc/l/2aVRDWx4k8TtTCGnAFB VC7ec6eqzKC08OrgEDauSScUB/8vd+2+w3kTonXYoySrMWpZ25EqHIpQ= X-Google-Smtp-Source: AGHT+IFYS2jqyDeOzBloLvZ0bZIFs65fckzCotqIKlqLE2xjHreCtZKjMMcY1hAOVb9mP16ToPnHHKzXUQxWTsbDXeI= X-Received: by 2002:a05:6820:2207:b0:5d5:a69c:fe68 with SMTP id 006d021491bc7-5da7c73957amr5011539eaf.4.1723670698431; Wed, 14 Aug 2024 14:24:58 -0700 (PDT) MIME-Version: 1.0 References: <20240802233801.GU10433@brightrain.aerifal.cx> <4c03697d-36fd-42e3-8a27-e121d3acc522@brad-house.com> <20240814211623.GL10433@brightrain.aerifal.cx> In-Reply-To: <20240814211623.GL10433@brightrain.aerifal.cx> From: enh Date: Wed, 14 Aug 2024 17:24:43 -0400 Message-ID: To: musl@lists.openwall.com Cc: Brad House Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [musl] [PATCH 1/1] IN6_IS_ADDR_LOOPBACK() and similar macros warn on -Wcast-qual 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=E2=80=AFPM Rich Felker 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] =3D= =3D 0xff) > > >> | ^ > > >>... ^ > > >> > > >>Full build output: https://github.com/c-ares/c-ares/actions/runs/1021= 9723015/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] =3D=3D 0 && ((uint32_t *) (a))[1] =3D= =3D 0 && \ > > >>- ((uint32_t *) (a))[2] =3D=3D 0 && \ > > >>- ((uint8_t *) (a))[12] =3D=3D 0 && ((uint8_t *) (a))[13] =3D= =3D 0 && \ > > >>- ((uint8_t *) (a))[14] =3D=3D 0 && ((uint8_t *) (a))[15] =3D= =3D 1 ) > > >>+ (((const uint32_t *) (a))[0] =3D=3D 0 && \ > > >>+ ((const uint32_t *) (a))[1] =3D=3D 0 && \ > > >>+ ((const uint32_t *) (a))[2] =3D=3D 0 && \ > > >>+ ((const uint8_t *) (a))[12] =3D=3D 0 && \ > > >>+ ((const uint8_t *) (a))[13] =3D=3D 0 && \ > > >>+ ((const uint8_t *) (a))[14] =3D=3D 0 && \ > > >>+ ((const uint8_t *) (a))[15] =3D=3D 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] =3D=3D 0 && ((uint32_t *) (a))[1] =3D= =3D 0 && \ > > - ((uint32_t *) (a))[2] =3D=3D 0 && ((uint32_t *) (a))[3] =3D= =3D 0) > > + (((a)->s6_addr32)[0] =3D=3D 0 && \ > > + ((a)->s6_addr32)[1] =3D=3D 0 && \ > > + ((a)->s6_addr32)[2] =3D=3D 0 && \ > > + ((a)->s6_addr32)[3] =3D=3D 0) > > > > #define IN6_IS_ADDR_LOOPBACK(a) \ > > - (((uint32_t *) (a))[0] =3D=3D 0 && ((uint32_t *) (a))[1] =3D= =3D 0 && \ > > - ((uint32_t *) (a))[2] =3D=3D 0 && \ > > - ((uint8_t *) (a))[12] =3D=3D 0 && ((uint8_t *) (a))[13] =3D= =3D 0 && \ > > - ((uint8_t *) (a))[14] =3D=3D 0 && ((uint8_t *) (a))[15] =3D= =3D 1 ) > > + (((a)->s6_addr32)[0] =3D=3D 0 && \ > > + ((a)->s6_addr32)[1] =3D=3D 0 && \ > > + ((a)->s6_addr32)[2] =3D=3D 0 && \ > > + ((a)->s6_addr)[12] =3D=3D 0 && \ > > + ((a)->s6_addr)[13] =3D=3D 0 && \ > > + ((a)->s6_addr)[14] =3D=3D 0 && \ > > + ((a)->s6_addr)[15] =3D=3D 1 ) > > > > -#define IN6_IS_ADDR_MULTICAST(a) (((uint8_t *) (a))[0] =3D=3D 0xff) > > +#define IN6_IS_ADDR_MULTICAST(a) (((a)->s6_addr)[0] =3D=3D 0xff) > > > > #define IN6_IS_ADDR_LINKLOCAL(a) \ > > - ((((uint8_t *) (a))[0]) =3D=3D 0xfe && (((uint8_t *) (a))[1] &= 0xc0) =3D=3D 0x80) > > + ((((a)->s6_addr)[0]) =3D=3D 0xfe && \ > > + (((a)->s6_addr)[1] & 0xc0) =3D=3D 0x80) > > > > #define IN6_IS_ADDR_SITELOCAL(a) \ > > - ((((uint8_t *) (a))[0]) =3D=3D 0xfe && (((uint8_t *) (a))[1] &= 0xc0) =3D=3D 0xc0) > > + ((((a)->s6_addr)[0]) =3D=3D 0xfe && \ > > + (((a)->s6_addr)[1] & 0xc0) =3D=3D 0xc0) > > > > #define IN6_IS_ADDR_V4MAPPED(a) \ > > - (((uint32_t *) (a))[0] =3D=3D 0 && ((uint32_t *) (a))[1] =3D= =3D 0 && \ > > - ((uint8_t *) (a))[8] =3D=3D 0 && ((uint8_t *) (a))[9] =3D=3D = 0 && \ > > - ((uint8_t *) (a))[10] =3D=3D 0xff && ((uint8_t *) (a))[11] = =3D=3D 0xff) > > + (((a)->s6_addr32)[0] =3D=3D 0 && \ > > + ((a)->s6_addr32)[1] =3D=3D 0 && \ > > + ((a)->s6_addr)[8] =3D=3D 0 && \ > > + ((a)->s6_addr)[9] =3D=3D 0 && \ > > + ((a)->s6_addr)[10] =3D=3D 0xff && \ > > + ((a)->s6_addr)[11] =3D=3D 0xff) > > > > #define IN6_IS_ADDR_V4COMPAT(a) \ > > - (((uint32_t *) (a))[0] =3D=3D 0 && ((uint32_t *) (a))[1] =3D= =3D 0 && \ > > - ((uint32_t *) (a))[2] =3D=3D 0 && ((uint8_t *) (a))[15] > 1) > > + (((a)->s6_addr32)[0] =3D=3D 0 && \ > > + ((a)->s6_addr32)[1] =3D=3D 0 && \ > > + ((a)->s6_addr32)[2] =3D=3D 0 && \ > > + ((a)->s6_addr)[15] > 1) > > > > #define IN6_IS_ADDR_MC_NODELOCAL(a) \ > > - (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) =3D= =3D 0x1)) > > + (IN6_IS_ADDR_MULTICAST(a) && \ > > + ((((a)->s6_addr)[1] & 0xf) =3D=3D 0x1)) > > > > #define IN6_IS_ADDR_MC_LINKLOCAL(a) \ > > - (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) =3D= =3D 0x2)) > > + (IN6_IS_ADDR_MULTICAST(a) && \ > > + ((((a)->s6_addr)[1] & 0xf) =3D=3D 0x2)) > > > > #define IN6_IS_ADDR_MC_SITELOCAL(a) \ > > - (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) =3D= =3D 0x5)) > > + (IN6_IS_ADDR_MULTICAST(a) && \ > > + ((((a)->s6_addr)[1] & 0xf) =3D=3D 0x5)) > > > > #define IN6_IS_ADDR_MC_ORGLOCAL(a) \ > > - (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) =3D= =3D 0x8)) > > + (IN6_IS_ADDR_MULTICAST(a) && \ > > + ((((a)->s6_addr)[1] & 0xf) =3D=3D 0x8)) > > > > #define IN6_IS_ADDR_MC_GLOBAL(a) \ > > - (IN6_IS_ADDR_MULTICAST(a) && ((((uint8_t *) (a))[1] & 0xf) =3D= =3D 0xe)) > > + (IN6_IS_ADDR_MULTICAST(a) && \ > > + ((((a)->s6_addr)[1] & 0xf) =3D=3D 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) = =3D=3D 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