Improve getsockname

Improve getsockname

[Bastien Roucariès]

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

Re: Improve getsockname

[Alejandro Colomar]

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

Re: Improve getsockname

[Bastien Roucariès]

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

struct sockaddr_storage, union (was: Improve getsockname)

[Alejandro Colomar]

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

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

[Bastien Roucariès]

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

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

[Alejandro Colomar]

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

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

[Bastien Roucariès]

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

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

[Alejandro Colomar]

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

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

[Alejandro Colomar]

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

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

[Bastien Roucariès]

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