Re: Improve getsockname

Alejandro Colomar
2023-01-19T12:45:11Z

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