Re: Improve getsockname

[Alejandro Colomar <alx.manpages@gmail.com>]

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