Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Pedro Alves <palves at redhat dot com>]

On 03/07/2015 06:52 PM, Eli Zaretskii wrote:
>> Date: Sat, 07 Mar 2015 18:20:18 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: gdb-patches@sourceware.org
>>
>> Those are BSD socket types, they've been this way ever since BSD
>> invented them.  The structs are not type compatible, even though
>> they have some common fields that are are put at the same offsets,
>> by design.
>>
>> See e.g.:
>>
>>   http://stackoverflow.com/questions/1429645/how-to-cast-sockaddr-storage-and-avoid-breaking-strict-aliasing-rules
> 
> IMO, the right way to handle this is to have our own struct (NOT
> union!) with the data, and fill the correct struct from it, field by
> field, when we need to pass it to a library function.

But that doesn't really make sense in this context.
These struct sockaddr_foo types aren't ours, they're pretty much
hard core to the sockets library.  You _need_ to fill in a
struct sockaddr_foo, and pass that to bind/accept, but disguised
as a "struct sockaddr".  That's just how those calls work.  It's
impossible to fill in a struct sockaddr with the data, because
struct sockaddr does not have the necessary fields.  E.g.:

(top-gdb) p sizeof (struct sockaddr)
$3 = 16
(top-gdb) p sizeof (struct sockaddr_in)
$2 = 16
(top-gdb) p sizeof (struct sockaddr_in6)
$1 = 28
(top-gdb) p sizeof (struct sockaddr_un)
$4 = 110

if 'struct sockaddr' HAD been defined like:

struct sockaddr {
    sa_family_t sa_family;
}

and then struct sockaddr_foos had been defined as "extending"
struct sockaddr, like e.g.:

struct sockaddr_in6 {
    /* Extend a sockaddr */
    struct sockaddr sa;

    /* ... and add more data */
    in_port_t sin6_port;
    uint32_t sin6_flowinfo;
    struct in6_addr sin6_addr;
    uint32_t sin6_scope_id;
}

That is, the first field of the structure had been the
abstract "struct sockaddr", then it'd be OK to cast struct sockaddr_foo
to "struct sockaddr".  C allows that.

(Another possibility would have been for bind/accept to
take a "sa_family_t *sa_family" as argument instead of
"struct sockaddr *", same thing.)

But, they're not.  That ship has sailed many years ago.
What we have is types like these (from gdb's 'ptype',
typedefs may be missing):

struct sockaddr {
    sa_family_t sa_family;
    char sa_data[14];
}

struct sockaddr_in {
    sa_family_t sin_family;
    in_port_t sin_port;
    struct in_addr sin_addr;
    unsigned char sin_zero[8];
}

struct sockaddr_in6 {
    sa_family_t sin6_family;
    in_port_t sin6_port;
    uint32_t sin6_flowinfo;
    struct in6_addr sin6_addr;
    uint32_t sin6_scope_id;
}

struct sockaddr_un {
    sa_family_t sun_family;
    char sun_path[108];
}

And thus casting between "struct sockaddr *"
and "struct sockaddr_in6 *" is invalid.  These types can't
alias, thus with this:

 struct sockaddr_in *addr_in = ...;
 struct sockaddr *addr = (struct sockaddr *) addr_in;

 addr->sa_family = 1;
 addr_in->sin_family = 2;
 if (addr->sa_family != 1) {
    abort ();
 }

the compiler is free to completely eliminate the 'if'.

> A union is just a type-cast in disguise; if using it is wrong, we
> shouldn't try.  (And what if one of these days GCC will acquire a
> capability to see through them?)
> 

As discussed in that stackexchange page, it won't.  It's
implementation-defined till C90 whether accessing a different union member
from the one last used to store a value was valid, a.k.a., type-punning.

In GCC's implementation (and others' I believe), it's valid.
Please see:

 https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation
and:
 https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Optimize-Options.html#Type-punning

And then the C99 standard changed its wording to make it valid everywhere.

See DR283 (defect report):
 http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm

And C99 TC3, section 6.5, paragraph 7:
 www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Casts are invalid.  Unions work in practice with old compilers,
and are required to work in "modern" (old by now) compilers.  So
really unions are the proper solution for this, however unfortunate.

>> It's not a C++ restriction.  The old code was invalid C code.
> 
> I'm talking about the new code.

Then I don't understand what you're saying, sorry.  This has nothing
to do with C++.  C++ happened to catch it due to the missing cast.  I
could have added the cast to that one place that missed it, but that
would just be propagating both invalid C++ _and_ invalid _C_ code.

Thanks,
Pedro Alves