Re: Improve getsockname

Bastien Roucariès
2023-01-19T19:45:01Z

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

}