[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: - Which caused Zack to update some old stackoverflow answer :) - He made me realize getnameinfo(3) exists, and so I used in place of a similar function that I had implemented recently: - And while reading the manual page for getnameinfo(3), I realized it was missing some missing _Nullable qualifiers: 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?= >>> 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 --