From owner-svn-src-all@FreeBSD.ORG Tue Aug 10 16:24:11 2010 Return-Path: Delivered-To: svn-src-all@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 58CF9106577A; Tue, 10 Aug 2010 16:24:11 +0000 (UTC) (envelope-from olli@fromme.com) Received: from haluter.fromme.com (haluter.fromme.com [212.17.241.231]) by mx1.freebsd.org (Postfix) with ESMTP id 903008FC1E; Tue, 10 Aug 2010 16:24:10 +0000 (UTC) Received: from haluter.fromme.com (irc_sucks@localhost [127.0.0.1]) by haluter.fromme.com (8.14.3/8.14.3) with ESMTP id o7AGNslM042681; Tue, 10 Aug 2010 18:24:01 +0200 (CEST) (envelope-from olli@fromme.com) Received: (from olli@localhost) by haluter.fromme.com (8.14.3/8.14.3/Submit) id o7AGNs7I042679; Tue, 10 Aug 2010 18:23:54 +0200 (CEST) (envelope-from olli) From: Oliver Fromme Message-Id: <201008101623.o7AGNs7I042679@haluter.fromme.com> To: imp@bsdimp.com (M. Warner Losh) Date: Tue, 10 Aug 2010 18:23:53 +0200 (CEST) In-Reply-To: <20100810.093656.167578749323544001.imp@bsdimp.com> X-Mailer: ELM [version 2.5 PL8] MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.1.2 (haluter.fromme.com [127.0.0.1]); Tue, 10 Aug 2010 18:24:01 +0200 (CEST) Cc: src-committers@FreeBSD.org, jilles@stack.nl, svn-src-all@FreeBSD.org, olli@FreeBSD.org, svn-src-head@FreeBSD.org, des@des.no Subject: Re: svn commit: r211023 - head/usr.sbin/syslogd X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Aug 2010 16:24:11 -0000 M. Warner Losh wrote: > In message: <201008101313.o7ADDhYE040900@haluter.fromme.com> > Oliver Fromme writes: > : M. Warner Losh wrote: > : > The casting that syslogd does with struct sockaddrFOO is what triggers > : > it. I'm not sure how to fix it, so there's about 6 or 8 programs in > : > the tree that have WARNS lowered to 3 because of it. > : > : I've given it try, please see the patch below. > : This is not really pretty, but it's a start. > : It builds with WARNS=6 on all archs, including mips. > : > : What do you think? Please ignore that patch. It compiles, but has some problems. des pointed out that it makes sense to use a macro for type casts. The new patch below does this for the cases where simply using struct sockaddr_storage doesn't work. > [...] > : @@ -2409,9 +2414,9 @@ > : continue; > : } > : reject = 0; > : - for (j = 0; j < 16; j += 4) { > : - if ((*(u_int32_t *)&sin6->sin6_addr.s6_addr[j] & *(u_int32_t *)&m6p->sin6_addr.s6_addr[j]) > : - != *(u_int32_t *)&a6p->sin6_addr.s6_addr[j]) { > : + for (j = 0; j < 16; j++) { > : + if ((sin6->sin6_addr.s6_addr[j] & m6p->sin6_addr.s6_addr[j]) > : + != a6p->sin6_addr.s6_addr[j]) { > > This code looks better, but why have the casts been removed? I take > it they aren't necessary? Right. On the other hand, the loop iterations are increased from 4 to 16, making the whole thing less efficient. In the new patch below I introduced a typecast macro for this case, too, so the iterations stay at 4. Again, the new patch passes the universe test. Best regards Oliver --- syslogd.c.orig 2010-08-05 21:59:11.000000000 +0200 +++ syslogd.c 2010-08-10 18:15:46.000000000 +0200 @@ -123,6 +123,15 @@ #define MAXUNAMES 20 /* maximum number of user names */ /* + * Macros to cast a struct sockaddr, or parts thereof. + * These are needed to silence the compiler on architectures + * with strict alignment requirements. + */ + +#define SIN_CAST(sa) ((struct sockaddr_in *)(uintptr_t)(sa)) +#define UINT32_CAST(c) (*(u_int32_t *)(uintptr_t)&(c)) + +/* * Unix sockets. * We have two default sockets, one with 666 permissions, * and one for privileged programs. @@ -330,8 +339,8 @@ static void readklog(void); static void reapchild(int); static void usage(void); -static int validate(struct sockaddr *, const char *); -static void unmapped(struct sockaddr *); +static int validate(struct sockaddr_storage *, const char *); +static void unmapped(struct sockaddr_storage *); static void wallmsg(struct filed *, struct iovec *, const int iovlen); static int waitdaemon(int, int, int); static void timedout(int); @@ -652,8 +661,8 @@ if (l > 0) { line[l] = '\0'; hname = cvthname((struct sockaddr *)&frominet); - unmapped((struct sockaddr *)&frominet); - if (validate((struct sockaddr *)&frominet, hname)) + unmapped(&frominet); + if (validate(&frominet, hname)) printline(hname, line, RemoteAddDate ? ADDDATE : 0); } else if (l < 0 && errno != EINTR) logerror("recvfrom inet"); @@ -678,17 +687,17 @@ } static void -unmapped(struct sockaddr *sa) +unmapped(struct sockaddr_storage *ss) { struct sockaddr_in6 *sin6; struct sockaddr_in sin4; - if (sa->sa_family != AF_INET6) + if (ss->ss_family != AF_INET6) return; - if (sa->sa_len != sizeof(struct sockaddr_in6) || - sizeof(sin4) > sa->sa_len) + if (ss->ss_len != sizeof(struct sockaddr_in6) || + sizeof(sin4) > ss->ss_len) return; - sin6 = (struct sockaddr_in6 *)sa; + sin6 = (struct sockaddr_in6 *)ss; if (!IN6_IS_ADDR_V4MAPPED(&sin6->sin6_addr)) return; @@ -699,7 +708,7 @@ sizeof(sin4.sin_addr)); sin4.sin_port = sin6->sin6_port; - memcpy(sa, &sin4, sin4.sin_len); + memcpy(ss, &sin4, sin4.sin_len); } static void @@ -1186,7 +1195,7 @@ break; case F_FORW: - port = (int)ntohs(((struct sockaddr_in *) + port = (int)ntohs((SIN_CAST (f->f_un.f_forw.f_addr->ai_addr))->sin_port); if (port != 514) { dprintf(" %s:%d\n", f->f_un.f_forw.f_hname, port); @@ -1711,7 +1720,7 @@ break; case F_FORW: - port = (int)ntohs(((struct sockaddr_in *) + port = (int)ntohs((SIN_CAST (f->f_un.f_forw.f_addr->ai_addr))->sin_port); if (port != 514) { printf("%s:%d", @@ -2340,7 +2349,7 @@ * Validate that the remote peer has permission to log to us. */ static int -validate(struct sockaddr *sa, const char *hname) +validate(struct sockaddr_storage *ss, const char *hname) { int i; size_t l1, l2; @@ -2369,8 +2378,8 @@ strlcat(name, ".", sizeof name); strlcat(name, LocalDomain, sizeof name); } - if (getnameinfo(sa, sa->sa_len, ip, sizeof ip, port, sizeof port, - NI_NUMERICHOST | NI_NUMERICSERV) != 0) + if (getnameinfo((struct sockaddr *)ss, ss->ss_len, ip, sizeof ip, port, + sizeof port, NI_NUMERICHOST | NI_NUMERICSERV) != 0) return (0); /* for safety, should not occur */ dprintf("validate: dgram from IP %s, port %s, name %s;\n", ip, port, name); @@ -2384,12 +2393,12 @@ } if (ap->isnumeric) { - if (ap->a_addr.ss_family != sa->sa_family) { + if (ap->a_addr.ss_family != ss->ss_family) { dprintf("rejected in rule %d due to address family mismatch.\n", i); continue; } if (ap->a_addr.ss_family == AF_INET) { - sin4 = (struct sockaddr_in *)sa; + sin4 = (struct sockaddr_in *)ss; a4p = (struct sockaddr_in *)&ap->a_addr; m4p = (struct sockaddr_in *)&ap->a_mask; if ((sin4->sin_addr.s_addr & m4p->sin_addr.s_addr) @@ -2400,7 +2409,7 @@ } #ifdef INET6 else if (ap->a_addr.ss_family == AF_INET6) { - sin6 = (struct sockaddr_in6 *)sa; + sin6 = (struct sockaddr_in6 *)ss; a6p = (struct sockaddr_in6 *)&ap->a_addr; m6p = (struct sockaddr_in6 *)&ap->a_mask; if (a6p->sin6_scope_id != 0 && @@ -2410,8 +2419,8 @@ } reject = 0; for (j = 0; j < 16; j += 4) { - if ((*(u_int32_t *)&sin6->sin6_addr.s6_addr[j] & *(u_int32_t *)&m6p->sin6_addr.s6_addr[j]) - != *(u_int32_t *)&a6p->sin6_addr.s6_addr[j]) { + if ((UINT32_CAST(sin6->sin6_addr.s6_addr[j]) & UINT32_CAST(m6p->sin6_addr.s6_addr[j])) + != UINT32_CAST(a6p->sin6_addr.s6_addr[j])) { ++reject; break; }