* Improve getsockname @ 2023-01-17 10:22 Bastien Roucariès 2023-01-19 12:42 ` Alejandro Colomar 0 siblings, 1 reply; 10+ messages in thread From: Bastien Roucariès @ 2023-01-17 10:22 UTC (permalink / raw) To: Michael Kerrisk (man-pages); +Cc: linux-man [-- Attachment #1.1: Type: text/plain, Size: 481 bytes --] Hi, I have a lot of student that does not use correctly getsockname in case of dual stack. 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. maybe forcing ENOTSUPP if *addr > sizeof(sockadd_storage) Regards Bastien [-- Attachment #1.2: 0001-Document-use-of-struct-sockaddr_storage-in-getsockna.patch --] [-- Type: text/x-patch, Size: 2699 bytes --] 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. +.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_* +type, according to the current protocol family, given by the member ss_family. .SH RETURN VALUE On success, zero is returned. On error, \-1 is returned, and @@ -80,10 +91,55 @@ For background on the .I socklen_t type, see .BR accept (2). +.PP +Security and portability wise, use of +.I struct sockaddr_storage +type as +.I addr +and +.I addrlen +set to +.BI "sizeof(struct sockaddr_storage)" +is strongly encouraged. Particularly this usage avoid bugs in dual stack IPv4+IPv6 configuration. +.PP +Historical use of +.I addr +requires one to use a structure specific to the protocol family in use, +such as +.I sockaddr_in +(AF_INET or IPv4), +.I sockaddr_in6 +(AF_INET6 or IPv6), or +.I sockaddr_un +(AF_UNIX) +cast to a +.I (struct sockaddr *). +The purpose of the +.I struct sockaddr * +type +is purely to allow casting of domain-specific socket address types to a +"generic" type, so as to avoid compiler warnings about type mismatches in calls to the sockets API. +Nevertheless, +. I struct sockaddr * +is too small to hold newer protocol address (for instance IPv6) and not always well aligned. +.PP +Even if +.I sockaddr_storage +type is large enough at compilation time to hold any of the +.I sockaddr_* +variants known by the library, +this guarantee will not hold for future. Newer protocol may need an increase of +.I sockaddr_storage +size or alignement requirement, and safe program should always runtime check if returned +.I addr +is smaller or equal to compile time +.BI "sizeof(struct sockaddr_storage)" +size. .SH SEE ALSO .BR bind (2), .BR socket (2), .BR getifaddrs (3), +.BR sockaddr_storage (3type), .BR ip (7), .BR socket (7), .BR unix (7) -- 2.39.0 [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: Improve getsockname 2023-01-17 10:22 Improve getsockname Bastien Roucariès @ 2023-01-19 12:42 ` Alejandro Colomar 2023-01-19 19:44 ` Bastien Roucariès 0 siblings, 1 reply; 10+ messages in thread From: Alejandro Colomar @ 2023-01-19 12:42 UTC (permalink / raw) To: Bastien Roucariès; +Cc: linux-man [-- Attachment #1.1: Type: text/plain, Size: 4611 bytes --] 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. > > 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. > 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. > +.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. Cheers, Alex > +type, according to the current protocol family, given by the member ss_family. > .SH RETURN VALUE > On success, zero is returned. > On error, \-1 is returned, and > @@ -80,10 +91,55 @@ For background on the > .I socklen_t > type, see > .BR accept (2). > +.PP > +Security and portability wise, use of > +.I struct sockaddr_storage > +type as > +.I addr > +and > +.I addrlen > +set to > +.BI "sizeof(struct sockaddr_storage)" > +is strongly encouraged. Particularly this usage avoid bugs in dual stack IPv4+IPv6 configuration. > +.PP > +Historical use of > +.I addr > +requires one to use a structure specific to the protocol family in use, > +such as > +.I sockaddr_in > +(AF_INET or IPv4), > +.I sockaddr_in6 > +(AF_INET6 or IPv6), or > +.I sockaddr_un > +(AF_UNIX) > +cast to a > +.I (struct sockaddr *). > +The purpose of the > +.I struct sockaddr * > +type > +is purely to allow casting of domain-specific socket address types to a > +"generic" type, so as to avoid compiler warnings about type mismatches in calls to the sockets API. > +Nevertheless, > +. I struct sockaddr * > +is too small to hold newer protocol address (for instance IPv6) and not always well aligned. > +.PP > +Even if > +.I sockaddr_storage > +type is large enough at compilation time to hold any of the > +.I sockaddr_* > +variants known by the library, > +this guarantee will not hold for future. Newer protocol may need an increase of > +.I sockaddr_storage > +size or alignement requirement, and safe program should always runtime check if returned > +.I addr > +is smaller or equal to compile time > +.BI "sizeof(struct sockaddr_storage)" > +size. > .SH SEE ALSO > .BR bind (2), > .BR socket (2), > .BR getifaddrs (3), > +.BR sockaddr_storage (3type), > .BR ip (7), > .BR socket (7), > .BR unix (7) > -- > 2.39.0 > -- <http://www.alejandro-colomar.es/> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: Improve getsockname 2023-01-19 12:42 ` Alejandro Colomar @ 2023-01-19 19:44 ` Bastien Roucariès 2023-01-19 20:19 ` struct sockaddr_storage, union (was: Improve getsockname) Alejandro Colomar 0 siblings, 1 reply; 10+ messages in thread From: Bastien Roucariès @ 2023-01-19 19:44 UTC (permalink / raw) To: Alejandro Colomar; +Cc: linux-man, eblake [-- Attachment #1.1: Type: text/plain, Size: 6076 bytes --] 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] Eric to be short posix behavior of sockaddr sockaddr_storage is UB and example supplied are UB. See here: https://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/socket.h.html 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. > > > > > 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. > > > 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; }; > > > +.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 > > Cheers, > > Alex > > > +type, according to the current protocol family, given by the member ss_family. > > .SH RETURN VALUE > > On success, zero is returned. > > On error, \-1 is returned, and > > @@ -80,10 +91,55 @@ For background on the > > .I socklen_t > > type, see > > .BR accept (2). > > +.PP > > +Security and portability wise, use of > > +.I struct sockaddr_storage > > +type as > > +.I addr > > +and > > +.I addrlen > > +set to > > +.BI "sizeof(struct sockaddr_storage)" > > +is strongly encouraged. Particularly this usage avoid bugs in dual stack IPv4+IPv6 configuration. > > +.PP > > +Historical use of > > +.I addr > > +requires one to use a structure specific to the protocol family in use, > > +such as > > +.I sockaddr_in > > +(AF_INET or IPv4), > > +.I sockaddr_in6 > > +(AF_INET6 or IPv6), or > > +.I sockaddr_un > > +(AF_UNIX) > > +cast to a > > +.I (struct sockaddr *). > > +The purpose of the > > +.I struct sockaddr * > > +type > > +is purely to allow casting of domain-specific socket address types to a > > +"generic" type, so as to avoid compiler warnings about type mismatches in calls to the sockets API. > > +Nevertheless, > > +. I struct sockaddr * > > +is too small to hold newer protocol address (for instance IPv6) and not always well aligned. > > +.PP > > +Even if > > +.I sockaddr_storage > > +type is large enough at compilation time to hold any of the > > +.I sockaddr_* > > +variants known by the library, > > +this guarantee will not hold for future. Newer protocol may need an increase of > > +.I sockaddr_storage > > +size or alignement requirement, and safe program should always runtime check if returned > > +.I addr > > +is smaller or equal to compile time > > +.BI "sizeof(struct sockaddr_storage)" > > +size. > > .SH SEE ALSO > > .BR bind (2), > > .BR socket (2), > > .BR getifaddrs (3), > > +.BR sockaddr_storage (3type), > > .BR ip (7), > > .BR socket (7), > > .BR unix (7) > > -- > > 2.39.0 > > > > -- > <http://www.alejandro-colomar.es/> > [-- Attachment #1.2: testgetsockname.c --] [-- Type: text/x-csrc, Size: 2041 bytes --] #include <sys/socket.h> #include <netinet/in.h> #include <arpa/inet.h> #include <sys/un.h> #include <stdio.h> #include <unistd.h> #include <errno.h> #include <stddef.h> union sockaddr_mayalias { sa_family_t ss_family; struct sockaddr sock; struct sockaddr_storage storage; struct sockaddr_in in; struct sockaddr_in6 in6; struct sockaddr_un un; }; int main() { union sockaddr_mayalias sa = {}; socklen_t addrlen = sizeof(sa); if(getsockname(STDIN_FILENO, &sa.sock, &addrlen) < 0) { perror("getsockname"); return 1; } if(addrlen >= sizeof(sa)) { errno = EPROTONOSUPPORT; perror("getsockname return a not supported sock_addr"); return 1; } switch(sa.ss_family) { case(AF_UNSPEC): printf("AF_UNSPEC socket\n"); break; case(AF_INET): { char s[INET_ADDRSTRLEN]; in_port_t port = ntohs(sa.in.sin_port); if (inet_ntop(AF_INET, &(sa.in.sin_addr), s, sizeof(s)) == NULL) { perror("inet_ntop"); return 1; } printf("AF_INET socket %s:%i\n",s,(int)port); break; } case(AF_INET6): { char s[INET6_ADDRSTRLEN]; in_port_t port = ntohs(sa.in6.sin6_port); if (inet_ntop(AF_INET6, &(sa.in6.sin6_addr), s, sizeof(s)) == NULL) { perror("inet_ntop"); return 1; } printf("AF_INET6 socket %s:%i\n",s,(int)port); break; } case(AF_UNIX): if(addrlen == sizeof(sa_family_t)) { printf("AF_UNIX socket anonymous\n"); break; } /* abstract */ if(sa.un.sun_path[0]=='\0') { printf("AF_UNIX abstract socket 0x"); for (int i = 0; i < (addrlen - sizeof(sa_family_t)); ++i) printf("%x",sa.un.sun_path[i]); printf("\n"); break; } /* named */ printf("AF_UNIX named socket "); for (int i=0; i < strnlen(sa.un.sun_path, addrlen - offsetof(struct sockaddr_un, sun_path));++i) printf("%c",sa.un.sun_path[i]); printf("\n"); break; default: errno = EPROTONOSUPPORT; perror("socket not supported"); return 1; } } [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* struct sockaddr_storage, union (was: Improve getsockname) 2023-01-19 19:44 ` Bastien Roucariès @ 2023-01-19 20:19 ` Alejandro Colomar 2023-01-19 21:00 ` Bastien Roucariès 0 siblings, 1 reply; 10+ messages in thread From: Alejandro Colomar @ 2023-01-19 20:19 UTC (permalink / raw) To: Bastien Roucariès; +Cc: linux-man, eblake, Zack Weinberg, GNU C Library [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: struct sockaddr_storage, union (was: Improve getsockname) 2023-01-19 20:19 ` struct sockaddr_storage, union (was: Improve getsockname) Alejandro Colomar @ 2023-01-19 21:00 ` Bastien Roucariès 2023-01-19 21:19 ` Alejandro Colomar 0 siblings, 1 reply; 10+ messages in thread From: Bastien Roucariès @ 2023-01-19 21:00 UTC (permalink / raw) To: Alejandro Colomar; +Cc: linux-man, eblake, Zack Weinberg, GNU C Library [-- 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 --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: struct sockaddr_storage, union (was: Improve getsockname) 2023-01-19 21:00 ` Bastien Roucariès @ 2023-01-19 21:19 ` Alejandro Colomar 2023-01-19 21:38 ` Bastien Roucariès 0 siblings, 1 reply; 10+ messages in thread From: Alejandro Colomar @ 2023-01-19 21:19 UTC (permalink / raw) To: Bastien Roucariès; +Cc: linux-man, eblake, Zack Weinberg, GNU C Library [-- Attachment #1.1: Type: text/plain, Size: 2935 bytes --] On 1/19/23 22:00, Bastien Roucariès wrote: [...] >> <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. Yes, many types have seen such additions at the end of it over time. In the Linux man-pages, I try to document all structures as "having at least these members", but may grow over time. > > 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; Such a field would make sense. In fact, I believe the Linux internal implementation of _un must have something similar, since it ensures null-termination even if the user passes a non-terminated string, IIRC. > }; > [...] >> 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. Nice; since it's backwards compatible, I'll (probably) suggest a patch for glibc. > >>>> >>>>> +.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. Sure. Will do. Cheers, Alex > > Bastien >> >> >> Cheers, >> >> Alex >> >> -- >> <http://www.alejandro-colomar.es/> >> > -- <http://www.alejandro-colomar.es/> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: struct sockaddr_storage, union (was: Improve getsockname) 2023-01-19 21:19 ` Alejandro Colomar @ 2023-01-19 21:38 ` Bastien Roucariès 2023-01-19 23:31 ` Alejandro Colomar 0 siblings, 1 reply; 10+ messages in thread From: Bastien Roucariès @ 2023-01-19 21:38 UTC (permalink / raw) To: Alejandro Colomar; +Cc: linux-man, eblake, Zack Weinberg, GNU C Library [-- Attachment #1: Type: text/plain, Size: 3562 bytes --] Le jeudi 19 janvier 2023, 21:19:49 UTC Alejandro Colomar a écrit : > > On 1/19/23 22:00, Bastien Roucariès wrote: > [...] > > >> <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. > > Yes, many types have seen such additions at the end of it over time. In the > Linux man-pages, I try to document all structures as "having at least these > members", but may grow over time. In fact it is not needed and it is the best argument of struct sockaddr_storage printf("%li %li",sizeof(struct sockaddr_storage),sizeof(struct sockaddr_un)); give me 128 vs 110... So if correctly documented and aliasing solved it will be the best of the world... Moreover kernel expect it https://elixir.bootlin.com/linux/latest/source/net/unix/af_unix.c#L293 > > > > > 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; > > Such a field would make sense. In fact, I believe the Linux internal > implementation of _un must have something similar, since it ensures > null-termination even if the user passes a non-terminated string, IIRC. > > > }; > > > [...] > > >> 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. > > Nice; since it's backwards compatible, I'll (probably) suggest a patch for glibc. > > > > >>>> > >>>>> +.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. > > Sure. Will do. > > Cheers, > > Alex > > > > > Bastien > >> > >> > >> Cheers, > >> > >> Alex > >> > >> -- > >> <http://www.alejandro-colomar.es/> > >> > > > > -- > <http://www.alejandro-colomar.es/> > [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: struct sockaddr_storage, union (was: Improve getsockname) 2023-01-19 21:38 ` Bastien Roucariès @ 2023-01-19 23:31 ` Alejandro Colomar 2023-01-20 0:12 ` Alejandro Colomar 2023-01-20 21:11 ` Bastien Roucariès 0 siblings, 2 replies; 10+ messages in thread From: Alejandro Colomar @ 2023-01-19 23:31 UTC (permalink / raw) To: Bastien Roucariès; +Cc: linux-man, eblake, Zack Weinberg, GNU C Library [-- Attachment #1.1: Type: text/plain, Size: 2493 bytes --] On 1/19/23 22:38, Bastien Roucariès wrote: > Le jeudi 19 janvier 2023, 21:19:49 UTC Alejandro Colomar a écrit : >> >> On 1/19/23 22:00, Bastien Roucariès wrote: >> [...] >> >>>> <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. >> >> Yes, many types have seen such additions at the end of it over time. In the >> Linux man-pages, I try to document all structures as "having at least these >> members", but may grow over time. > > In fact it is not needed and it is the best argument of struct sockaddr_storage > printf("%li %li",sizeof(struct sockaddr_storage),sizeof(struct sockaddr_un)); > give me 128 vs 110... > > So if correctly documented and aliasing solved it will be the best of the world... > > Moreover kernel expect it https://elixir.bootlin.com/linux/latest/source/net/unix/af_unix.c#L293 However... Considering that most APIs use `struct sockaddr *`, this wouldn't allow the internal libc implementation of functions like getnameinfo(3) to be free of UB. Maybe a better thing would be to do the following: struct sockaddr { union { struct { sa_family_t sa_family; char sa_data[]; }; struct sockaddr_in sin; struct sockaddr_in6 sin6; struct sockaddr_un sun; }; }; struct sockaddr_storage { union { sa_family_t ss_family; struct sockaddr sa; }; }; This makes sockaddr and sockaddr_storage have the same size, and also both can alias any of the children types through the unions, so one can use either of them for the same purpose. I'll be sending a patch soon for discussion. Cheers, Alex > >> >>> >>> 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; >> -- <http://www.alejandro-colomar.es/> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: struct sockaddr_storage, union (was: Improve getsockname) 2023-01-19 23:31 ` Alejandro Colomar @ 2023-01-20 0:12 ` Alejandro Colomar 2023-01-20 21:11 ` Bastien Roucariès 1 sibling, 0 replies; 10+ messages in thread From: Alejandro Colomar @ 2023-01-20 0:12 UTC (permalink / raw) To: Bastien Roucariès; +Cc: linux-man, eblake, Zack Weinberg, GNU C Library [-- Attachment #1.1: Type: text/plain, Size: 275 bytes --] While implementing this, I'm having some doubts about strict aliasing rules and function boundaries. You may be able to answer this question that I just posted: <https://software.codidact.com/posts/287748> Cheers, Alex -- <http://www.alejandro-colomar.es/> [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: struct sockaddr_storage, union (was: Improve getsockname) 2023-01-19 23:31 ` Alejandro Colomar 2023-01-20 0:12 ` Alejandro Colomar @ 2023-01-20 21:11 ` Bastien Roucariès 1 sibling, 0 replies; 10+ messages in thread From: Bastien Roucariès @ 2023-01-20 21:11 UTC (permalink / raw) To: Alejandro Colomar; +Cc: linux-man, eblake, Zack Weinberg, GNU C Library [-- Attachment #1: Type: text/plain, Size: 1672 bytes --] Le jeudi 19 janvier 2023, 23:31:09 UTC Alejandro Colomar a écrit : Hi, > On 1/19/23 22:38, Bastien Roucariès wrote: > > However... Considering that most APIs use `struct sockaddr *`, this wouldn't > allow the internal libc implementation of functions like getnameinfo(3) to be > free of UB. libc is safe thanks to the transparent union of pointer. > Maybe a better thing would be to do the following: > > > struct sockaddr { > union { > struct { > sa_family_t sa_family; > char sa_data[]; > }; > struct sockaddr_in sin; > struct sockaddr_in6 sin6; > struct sockaddr_un sun; > }; > }; > > struct sockaddr_storage { > union { > sa_family_t ss_family; > struct sockaddr sa; > }; > }; > No I think we could do better with recent C: struct osockaddr { unsigned short int sa_family; unsigned char sa_data[14]; char _extra[]; }; > This makes sockaddr and sockaddr_storage have the same size, and also both can > alias any of the children types through the unions, so one can use either of > them for the same purpose. We could not due to old talk protocol Only storage is free > > I'll be sending a patch soon for discussion. > > Cheers, > > Alex > > > > >> > >>> > >>> 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; > >> > > -- > <http://www.alejandro-colomar.es/> > [-- Attachment #2: This is a digitally signed message part. --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-01-20 21:11 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-01-17 10:22 Improve getsockname Bastien Roucariès 2023-01-19 12:42 ` Alejandro Colomar 2023-01-19 19:44 ` Bastien Roucariès 2023-01-19 20:19 ` struct sockaddr_storage, union (was: Improve getsockname) Alejandro Colomar 2023-01-19 21:00 ` Bastien Roucariès 2023-01-19 21:19 ` Alejandro Colomar 2023-01-19 21:38 ` Bastien Roucariès 2023-01-19 23:31 ` Alejandro Colomar 2023-01-20 0:12 ` Alejandro Colomar 2023-01-20 21:11 ` Bastien Roucariès
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).