svn commit: r211023 - head/usr.sbin/syslogd

[Jilles Tjoelker]

On Wed, Aug 11, 2010 at 07:05:53AM +1000, Bruce Evans wrote:
> On Tue, 10 Aug 2010, M. Warner Losh wrote:
> 
> > In message: <86sk2m1hsj.fsf at ds4.des.no>
> >            Dag-Erling Smørgrav <des at des.no> writes:
> > : "M. Warner Losh" <imp at bsdimp.com> writes:
> > : > /*
> > : >  * 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.
> > : >  */
> > :
> > : That's a bit harsh on the compiler, don't you think?  It never pays to
> > : hurt the compiler's feelings :)
> >
> > /*
> > * Macros to cast a struct sockaddr, or parts thereof.  struct
> > * sockaddr's alginment is loose to later be cast to a sockaddr_in or
> > * sockaddr_in6.  On architectures with strict alignment requirements,
> > * this leads to compiler warnings because the compiler doesn't know
> > * the ABI guarantees proper alignment.
> > */
> >
> > But this leads me to think that the right fix might be:
> >
> > /*
> > * Structure used by kernel to store most
> > * addresses.
> > */
> > struct sockaddr {
> > 	unsigned char	sa_len;		/* total length */
> > 	sa_family_t	sa_family;	/* address family */
> > 	char		sa_data[14];	/* actually longer; address value */
> > } __aligned(4);
> >
> > since that's what the ABI defines....

> That's almost what I said to do a day or two ago.  Not sure if 4 is correct.
> The amd64 and i386 ABI's only requires 1.  arm may (bogusly) require 4.

> The (POSIX) API requires struct sockaddr_storage to sufficient alignment
> for various things, including specifically casting of pointers to it
> into pointers to protocol-specific address structures, but it doesn't
> require this for struct sockaddr or stuct sockaddr_un.  I checked a
> later version (2007 draft).  Its requirement to cast struct sockaddr_un
> to struct sockaddr (NB: not the pointers) hasn't changed, and makes a
> little more sense to me now.  I don't see how such a cast can succeed
> (unless the types are the same), but casting the pointers cannot work
> without any alignment requirements, while casting of whole objects can
> in some cases.

> Testing shows that casting a struct sockaddr_un to a struct sockaddr
> doesn't work at all, as expected:

> % gcc:
> % z.c: In function `foo':
> % z.c:10: error: conversion to non-scalar type requested
> % 
> % TenDRA (for C90 only) is much more verbose:
> % "z.c", line 10: Error:
> %   [ISO C90 6.3.4]: Can't cast to the non-scalar type 'struct sockaddr'.
> %   [ISO C90 6.3.4]: Illegal conversion from type 'struct sockaddr_un' to type 'struct sockaddr'.
> %   [ISO C90 6.3.4]: Can't perform this conversion using an explicit cast.

> This shows that structs cannot be cast even to the same type.  Assignment
> of a struct sockaddr_un to a struct sockaddr would work if these types are
> the same, but that is not useful.  POSIX's requirement is unimplementable.

I think POSIX intended pointers to be cast, not the structures
themselves. (In fact, there is one sentence about struct
sockaddr_storage (in XBD 13 Headers <sys/socket.h>) that gets it right.)

Then, POSIX's requirements can be summarized as

struct sockaddr_storage * ---> { PS * } ---> struct sockaddr *

where PS are protocol-specific sockaddr structures.

Casts from left to right must be possible and must preserve the family
field. So it seems that adding an alignment requirement to struct
sockaddr requires that all other sockaddr structures have at least that
alignment requirement.

This may also require that the implementation use non-standard tricks to
avoid strict-aliasing problems.

This leaves casting a struct sockaddr * from getaddrinfo() to the
appropriate protocol family's sockaddr as "does not necessarily work".
Perhaps it is even sufficient that bind() and connect() accept
getaddrinfo()'s pointers, so that the cast may indeed fail (the kernel
copyin() imposes no alignment restriction).

-- 
Jilles Tjoelker