Re: Improve getsockname

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

[-- 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 --]