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

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

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

Hi Bastien,

On 1/21/23 14:30, Bastien Roucariès wrote:

[...]

>> Ahh, indeed it seems to be UB.  It's in the same 6.5.2.3/6:  there's a
>> requirement that the information about the union is kept in the function in
>> which it's accessed.
>>
>> The standard presents an example, which is a bit ambiguous:
>>
>>        The following is not a valid fragment (because the union type is not
>> visible within function f):
>>

[...]

>>
>> I don't know what's the intention if the union type was global but the variable
>> `u` was still not seen by f().  But it seems GCC's interpretation is UB,
>> according to the test we just saw.
>>
>> The solution that I can see for that is making sockaddr also be a union.  That
>> way, the union is kept across all calls (since they all use sockaddr).
>>
>> struct sockaddr {
>> 	union {
>> 		struct {
>> 			sa_family_t  sa_family;
>> 			char         sa_data[14];  // why 14?
>> 		}
>> 		struct sockaddr_in   sin;
>> 		struct sockaddr_in6  sin6;
>> 		struct sockaddr_un   sun;
>> 	};
>> };
> 
> No the solution is to avoid sockaddr and mark as deprecated.

Declaring `sockaddr` as deprecated means deprecating also:

accept(2)
accept4(2)
bind(2)
connect(2)
getpeername(2)
getsockname(2)
recvfrom(2)
sendto(2)
getnameinfo(3)

which use the type in their prototype.

Also, other types such as `addrinfo`, which contain `sockaddr` would also need 
to be deprecated, which would itself require deprecating:

getaddrinfo(3)
freeaddrinfo(3)

Since addrinfo is itself contained in other structures such as `gaicb`, we would 
also need to deprecate those, which would in turn require deprecating more APIs:

getaddrinfo_a(3)
gai_error(3)
gai_cancel(3)

And maybe I left some.  This feels like nuking the entire networking API, which 
I don't see happening soon.


Otherwise, we need to come up with a solution that keeps these APIs compatible 
with whatever new type we suggest using.  Changing them to use `void*` instead 
of `sockaddr*` would be possible, but would decrease type safety considerably, 
so there should be a good reason for that.

Suggesting to use always `sockaddr_storage` but using `sockaddr` in the function 
parameters keeps the current not-so-nice casting issues, which are not Undefined 
Behavior per se, but not ideal either (in fact, I don't think `void*` is much 
worse than code full of casts).  And it would also be error-prone, since users 
could get the idea that `sockaddr` can be used safely, since it's what gets 
passed as the parameter.

> The problem it should be part of union without raising a warning each time we use a safe type...

I don't understand this; please detail.

> 
> The other solution is to render public  and ABI stable the type here
> https://github.com/bminor/glibc/blob/ae612c45efb5e34713859a5facf92368307efb6e/socket/sys/socket.h#L78
> under for instance sockaddr_ptr and sockaddr_const_ptr

[[gnu::transparent_union]] (used in that link) seems like a "the design of this 
interface is bad, sorry, this workaround will just make it work".  I guess it 
just works, and probably it's the reason that so much undefined behavior hasn't 
exploded so far.  However, if we can solve this using core language features, 
I'd go that way.

> 
> Moreover this are some patch arch by arch
> https://sourceware.org/legacy-ml/libc-alpha/2016-02/msg00340.html that should be made default
> 
> Bastien
> 
> 
> 
>>
>> struct sockaddr_storage {
>> 	union {
>> 		sa_family_t          ss_family;
>> 		struct sockaddr      sa;
>> 	};
>> };

Hmm, this isn't still perfect.  Since the APIs get the sockaddr, this union 
information would be lost.  `sockaddr` needs to be the type that is declared. 
`sockaddr_storage` should just die; there's no way to make it compatible with 
APIs getting a `sockaddr`.  The attribute `transparent_union` is the only way to 
use is safely.

Cheers,

Alex

>>
>>
>> With this, sockaddr_storage becomes useless, but still usable.  New code, could
>> just use sockaddr and use the internal union members as necessary.  Union info
>> is kept across all function boundaries.

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

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