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

[Pedro Alves <palves@redhat.com>]

On 03/09/2015 10:45 AM, Pedro Alves wrote:
> 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.

With all that said, I'm having second thoughts on this ...  Per
6.3.2.3 ("A pointer to an object or incomplete type may be converted to
a pointer to a different object or incomplete type"), as long as we don't
actually access the fields of sockaddr through the "struct sockaddr *" pointer,
then the cast should be OK.  In the bind/accept cast, it should be the
internals of those functions that need the union trick.  Gosh, what a mess.

So I'll revert the union patch, and apply the original one that added
the missing cast to tracepoint.c...

Thanks,
Pedro Alves