struct sockaddr_storage, union (was: Improve getsockname)

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

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

[CC += Zack, glibc]

Hi Bastien, Eric, and Zack,

On 1/19/23 20:44, Bastien Roucariès wrote:
> Le jeudi 19 janvier 2023, 12:42:52 UTC Alejandro Colomar a écrit :
> Hi all,
> [adding Eric Blake from redhat and if I remember well member of POSIX comitee]

Thanks.

BTW, your post triggered some changes.  I didn't CC to not be too noisy, but 
since you seem to be interested in it, here are a few things:


-  Suggest deprecating struct sockaddr_storage to glibc:

 
<https://inbox.sourceware.org/libc-alpha/0f25d60f-f183-b518-b6c1-6d46aa63ee57@gmail.com/T/>

-  Which caused Zack to update some old stackoverflow answer :)

    <https://stackoverflow.com/a/42190913/6872717>

-  He made me realize getnameinfo(3) exists, and so I used in place of a similar 
function that I had implemented recently:

    <https://github.com/shadow-maint/shadow/pull/629>

-  And while reading the manual page for getnameinfo(3), I realized it was 
missing some missing _Nullable qualifiers:

 
<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=39017f90644141a8bd297c783201e04b238769fb>


BTW, Zack, glibc could add [[nonnull(1)]] for the prototype.  Should I send a 
patch adding it to the header?

> 
> Eric to be short posix behavior of sockaddr sockaddr_storage is UB

Agree.  Almost all uses of struct sockaddr_storage are UB, and those that are 
not UB, are error prone.  stockaddr_storage is bad.

> and example supplied are UB. See here:
> https://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/socket.h.html

I don't see any usage examples there.  Only a definition.  The definition isn't 
bad in itself; it's just that it's unusable.

BTW, now we need to check all the man pages that refer to that structure and fix 
them to not promote UB.

> 
> I think we could save the day by defining struct sockadd_storage using anonymous union (thanks to C11 support),
>   but it will leave C++. But how can be done...
> 
>> Hi Bastien,
>> Hi Bastien,
>>
>> On 1/17/23 11:22, Bastien Roucariès wrote:
>>> Hi,
>>>
>>> I have a lot of student that does not use correctly getsockname in case of
>>> dual stack.
>>
>> Please show some examples of common mistakes, if you can.
> oh the basic one is to pass a sockaddr_in instead of sockaddr_in6... Or to pass a sockaddr_in6 and do not think that it could be returned an IPV4 socket or an Unix stream socket.

Understood.

>>
>>>
>>> May be this kind of discussion should be factorized in  sockaddr_storage (the
>>> historical note particularly).
>>>
>>> i suppose the same should be done for getpeername
>>>
>>> I think a safe programming example may be given that accept a socket as stdin
>>> and print information on it. Using socat it could be simple to test.
>>
>> Please provide some if you can.  However, be careful, since it might easily fall
>> into Undefined Behavior.
> 
> Ok see it attached.

Thanks.

>>
>>> maybe
>>> forcing ENOTSUPP if *addr > sizeof(sockadd_storage)
>>>
>>> Regards
>>>
>>> Bastien
>>
>> I'll add some comments to the patch.
>>
>>>  From 0afb3ad23f8ea09331f21a377e3ad19c44e4df18 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Bastien=20Roucari=C3=A8s?= <rouca@debian.org>
>>> Date: Tue, 17 Jan 2023 10:07:43 +0000
>>> Subject: [PATCH] Document use of struct sockaddr_storage in getsockname
>>>
>>> Document:
>>> - storage requierement
>>> - future compatibility
>>> ---
>>>   man2/getsockname.2 | 56 ++++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 56 insertions(+)
>>>
>>> diff --git a/man2/getsockname.2 b/man2/getsockname.2
>>> index e6e8980c9..5914c9e12 100644
>>> --- a/man2/getsockname.2
>>> +++ b/man2/getsockname.2
>>> @@ -39,6 +39,17 @@ The returned address is truncated if the buffer provided is too small;
>>>   in this case,
>>>   .I addrlen
>>>   will return a value greater than was supplied to the call.
>>> +.PP
>>> +For greater portability
>>> +.I addr
>>> +should point to a structure of type
>>> +.I sockaddr_storage.
>>
>> This is not correct.  If the object has a type of 'struct sockaddr_storage',
>> then it can only be accessed as a 'struct sockaddr_storage'.  Trying to access
>> it as any other structure type would be Undefined Behavior.  The way to do it
>> conforming to ISO C would be to declare a union which contains all the 'struct
>> sockaddr_*' that are interesting, and access it through the correct union
>> member.  Anything else is on UB land.  The only use of 'struct sockaddr_storage'
>> that I can think of, is for having a more consistent union size.
> 
> Ok I see, Eric could we redefine at posix level the struct sockaddr_storage to be something like, it seems that it is allowed by
> /*
>   *  Following fields are implementation-defined.
>   */
> 
> union sockaddr_storage {
>    sa_family_t     ss_family;
>    struct sockaddr sock;
>    struct _sockaddr_storage storage;
>    struct sockaddr_in in;
>    struct sockaddr_in6 in6;
>    struct sockaddr_un un;
> };

Hmmm, interesting.  We can't do that, because this would be a breaking change to 
source code.  We can't change from `struct sockaddr_storage` to `union 
sockaddr_storage`.  However, this suggestion, combined with the idea of using an 
anonymous union, can be made to work as something that is compatible with the 
current sockaddr_storage:

struct sockaddr_storage {
	union {
		sa_family_t          ss_family;
		struct sockaddr      sa;
		struct sockaddr_in   sin;
		struct sockaddr_in6  sin6;
		struct sockaddr_un   sun;
	};
};

This is compatible:

-  It had at least the `ss_family` field.  It's still there, at the same binary 
location.
-  It has a size at least as large as any other sockaddr_* structure, and a 
suitable alignment.
-  Old code still works with it just fine.
-  New code will be able to avoid UB, and all casts, just by accessing the right 
structure element.
-  It's trivial to test at configure time if the implementation provides this 
new definition of the structure.

>>
>>> +.I sockaddr_storage
>>> +structure is large enough to hold any of the other
>>> +.I sockaddr_*
>>> +variants and always well aligned. On return, it should be cast to the correct
>>> +.I sockaddr_*
>>
>> The fact that it is correctly aligned, and a cast will work most of the time,
>> isn't enough for strict aliasing rules.  The compiler is free to assume things,
>> just by the fact that it's a different type.
> 
> Ok any idea for writing this kind of stuff

I'm thinking about writing something to several pages; will keep you all updated 
on important changes to the pages.


Cheers,

Alex

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

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