svn commit: r211023 - head/usr.sbin/syslogd
M. Warner Losh
imp at bsdimp.com
Tue Aug 10 17:38:53 UTC 2010
In message: <201008101623.o7AGNs7I042679 at haluter.fromme.com>
Oliver Fromme <olli at fromme.com> writes:
:
: M. Warner Losh wrote:
: > In message: <201008101313.o7ADDhYE040900 at haluter.fromme.com>
: > Oliver Fromme <olli at fromme.com> 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
More information about the svn-src-all
mailing list