From owner-svn-src-head@FreeBSD.ORG Tue Aug 10 17:38:56 2010 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 9C70D106567E for ; Tue, 10 Aug 2010 17:38:56 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (bsdimp.com [199.45.160.85]) by mx1.freebsd.org (Postfix) with ESMTP id 054878FC14 for ; Tue, 10 Aug 2010 17:38:55 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.14.3/8.14.1) with ESMTP id o7AHYt2N076558; Tue, 10 Aug 2010 11:34:55 -0600 (MDT) (envelope-from imp@bsdimp.com) Date: Tue, 10 Aug 2010 11:06:42 -0600 (MDT) Message-Id: <20100810.110642.335141733495090585.imp@bsdimp.com> To: olli@fromme.com From: "M. Warner Losh" In-Reply-To: <201008101623.o7AGNs7I042679@haluter.fromme.com> References: <20100810.093656.167578749323544001.imp@bsdimp.com> <201008101623.o7AGNs7I042679@haluter.fromme.com> X-Mailer: Mew version 6.3 on Emacs 22.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit 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-head@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 10 Aug 2010 17:38:56 -0000 In message: <201008101623.o7AGNs7I042679@haluter.fromme.com> Oliver Fromme writes: : : 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)) You might want to add an explanation that the ABI guarantees the uint32's will have proper alignment to be accessed in this way. Maybe: /* * Macros to cast a struct sockaddr, or parts thereof. * On architectures with strict alignment requirements, the compiler * can bogusly warn about alignment problems since its static analysis * is insufficient for it to know that with the APIs used, there * really is no alignment issue. */ : +/* : * 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; : } : : Why 16 and 4 here? What's so magical about them? Warner