Re: [PATCH v2] socket: Implement sockaddr_storage with an anonymous union

[Alejandro Colomar <alx.manpages@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3179 bytes --]

Hi Zack,

On 1/20/23 20:25, Alejandro Colomar wrote:
> [CC += GCC]  // pun not intended :P
> 
> Hi Zack,
> 
> On 1/20/23 19:04, Zack Weinberg wrote:
>> On Fri, Jan 20, 2023, at 8:40 AM, Alejandro Colomar wrote:
>>> The historical design of `sockaddr_storage` makes it impossible to use
>>> without breaking strict aliasing rules.  Not only this type is unusable,
>>> but even the use of other `sockaddr_*` structures directly (when the
>>> programmer only cares about a single address family) is also incorrect,
>>> since at some point the structure will be accessed as a `sockaddr`, and
>>> that breaks strict aliasing rules too.
>>>
>>> So, the only way for a programmer to not invoke Undefined Behavior is to
>>> declare a union that includes `sockaddr` and any `sockaddr_*` structures
>>> that are of interest, which allows later accessing as either the correct
>>> structure or plain `sockaddr` for the sa_family.
>>
>> ...
>>
>>>      struct new_sockaddr_storage  nss;
>>>
>>>      // ... (initialize oss and nss)
>>>
>>>      inet_sockaddr2str(&nss.sa);  // correct (and has no casts)
>>
>> I think we need to move slowly here and be _sure_ that any proposed change
>> does in fact reduce the amount of UB.
> 
> Sure, I'm just sending the patch to polish the idea around some concrete code. 
> While I checked that it compiles, I didn't add any tests about it or anything, 
> to see that it's usable (and Joseph's email showed me that it's far from being 
> finished).  I expect that this'll take some time.
> 
> 
>>  This construct, in particular, might
>> not actually be correct in practice: see https://godbolt.org/z/rn51cracn for
>> a case where, if I'm reading it right, the compiler assumes that a write
>> through a `struct fancy *` cannot alter the values accessible through a
>> `struct simple *` even though both pointers point into the same union.
>> (Test case provided by <https://stackoverflow.com/users/363751/supercat>;
> 

[...]

I was wrong in my guess; the correct output is 3/3; I think I had read it the 
other way around.  So yes, I believe it's doing what you just wrote there, but 
can't understand why.

I reduced @supercat's example to a smaller reproducer program (I couldn't 
minimize it any more than this; any further simplification removes the incorrect 
behavior):

#include <stdio.h>

struct a { int y[1];};
struct b { int y[1];};
union u  { struct a a; struct b b; };


int read_a(struct a *a)
{
     return a->y[0];
}

void write_b(struct b *b, int j)
{
     b->y[j] = 2;
}

int use_union(union u *u, int j)
{
     if (u->a.y[0] == 0)
         write_b(&u->b, j);
         //write_b((struct b *)u, j);   // this has the same issue
     return read_a(&u->a);
     return read_a((struct a *)u);      // this has the same issue
}

int (*volatile vtest)(union u *u, int j) = use_union;

int main(void)
{
     int       r1, r2;
     union u   u;
     struct b  b = {0};

     u.b = b;
     r1 = vtest(&u, 0);
     r2 = u.a.y[0];

     printf("%d/%d\n", r1, r2);
}


Cheers,

Alex

-- 
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]