Re: struct sockaddr_storage, union (was: Improve getsockname)

["Bastien Roucariès" <rouca@debian.org>]

[-- Attachment #1: Type: text/plain, Size: 8386 bytes --]

Le jeudi 19 janvier 2023, 20:19:31 UTC Alejandro Colomar a écrit :
> [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/>

I do not believe it is broken by design. It should be used with care and warning. 

BTW if we go to the anonymous union way could we add at the end a _null_reserved_field. It will help for unix socket and the infamous sun_path could not end with null...
May be it is too late from an ABI point of view, but for me the posix contract from an ABI point of view is that I said in the note  sockaddr_storage  could grow but not be reduced.

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

sockaddr_storage will do the right thing for sun_path if we initalize with
stuct sockaddr_storage addr = {}

> 
> -  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...

Ok c++17 allow anonymous union so it is fixable....


> > 
> >> 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;
> 	};
> };


Yes if as I said if we could add a __null_reserved field it will help for un...

> 
> 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 agree I could even add a macro for autoconf-archive (I am upstream) and post a patch for gnulib.

> >>
> >>> +.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.

Please exchange with me... It is really a pitffall for my student, so I could help here.

Bastien
> 
> 
> Cheers,
> 
> Alex
> 
> -- 
> <http://www.alejandro-colomar.es/>
> 


[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]