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

[Bruce Evans]

On Mon, 9 Aug 2010, Jilles Tjoelker wrote:


> On Sun, Aug 08, 2010 at 03:36:08PM -0600, 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.
>
> This problem is common with programs that use getaddrinfo() and then
> look into the address family specific parts of the address (it was
> probably not the intention of the getaddrinfo() API to do this very
> often). Obviously, when ai_family is the corresponding value, the
> ai_addr really points to that particular kind of sockaddr, but gcc only
> knows that struct sockaddr_in and struct sockaddr_in6 have a higher
> alignment requirement than struct sockaddr.

POSIX even standardized this broken (unimplementable in C) API.  AT
least in the old 2001 draft 7, it says that sockaddr_un shall include
much the same members that FreeBSD's version has, and that applications
shall access these in an even more broken way than by casting the
pointer: it says that the struct type sockaddr_un shall be cast to
struct access for use with the socket functions.  I don't even remember
seeing code so broken as to expect casting whole structs to an
incompatible type to work.  However, this is not completely
unimplementable -- it just requires the struct types to be compatible,
i.e., the same.  But the reason for existence of sockaddr_un is to have
the struct types not the same.  They are not the same in FreeBSD.  The
pointers can probably made compatible enough in practice by implementing
them in non-C using sufficient alignment and/or packing directives.
Without these, the alignment and packing for the structs could easily
end up incompatible due to gcc wanting to align the larger array of
char more than the smaller array of char, like it does for global
arrays of char.  ABIs may already prevent such extra alignment in
practice.

> In some cases, the address may already be copied, and making the
> destination the family-based type or struct sockaddr_storage (which has
> a high alignment requirement) will avoid problems.
>
> In other cases, I propose adding a cast to void * in between, like
>  (struct sockaddr_in *)(void *)ai->ai_addr
>
> Note that this workaround must not be applied mindlessly to avoid
> -Wcast-align, but only when it is known from other information that the
> object really has that type.

Of course it just breaks the warning.

> If you don't like this workaround, perhaps copy the data to a variable
> of the correct type. Addresses are not particularly big so the
> performance hit should not be bad.
>
> It is unfortunate to have to miss other warnings because of this.

Copying is just a fancier way of breaking the warning in this case.

I was thinking of packing and aligning everything in the struct to ensure
that the struct layouts are identical in their common part, but to just
break the warning in a way that is easy to undo, it might be enough to
declare the whole structs as __aligned(1).  The structs consist of
u_chars (sometimes spelled uint8_t and sometimes spelled char), so their
natural alignment is 1.  But I just remember that arm uses unnattural
alignment.  __aligned(4) might need to be used for it to preserve the ABI.
What is the struct layout for arm?

Bruce