Re: struct sockaddr_storage

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

[-- Attachment #1.1: Type: text/plain, Size: 3639 bytes --]

Hi Zack!

On 1/19/23 16:40, Zack Weinberg via Libc-alpha wrote:
> On Thu, Jan 19, 2023, at 9:11 AM, Alejandro Colomar via Libc-alpha wrote:
>> I just received a report about struct sockaddr_storage in the man pages.  It
>> reminded me of some concern I've always had about it: it doesn't seem to be a
>> usable type.
> 
> Yeah, struct sockaddr_storage has only one genuine use case in my experience: to
> know how big to make the buffer for a call to `getpeername` (or, equivalently,
> `accept` with non-null second argument) *when you know that the only address
> families you care about are IPv4 and IPv6*. And you can do that just as well with
> 
> union {
>    struct sockaddr sa;
>    struct sockaddr_in sa_in;
>    struct sockaddr_in6 sa_in6;
> } addrbuf;
> 
> which also means fewer aliasing issues.

And also less chance of accidentally confusing someone to believe that it's just 
fine if they later add _un while still relying on _storage.

So, even for the only non-broken use of _storage, I'd argue it should be 
avoided, as something error-prone.

> 
> I'd suggest that the manpages should say:
> 
> 0. It is almost never the Right Thing to declare a variable of type `struct sockaddr` or `struct sockaddr_storage`.

Agree.

> 1. To the maximum extent possible, rely on `getaddrinfo` and `getnameinfo` to deal with socket addresses for you.  Treat them as opaque.

All this made me realize I fell into this UB trap recently :)

<https://github.com/shadow-maint/shadow/pull/617/files>
See my definition of a function called inet_sockaddr2str(), which of course has 
UB (it's impossible to define such a function without relying on it, I believe; 
glibc's own getnameinfo(3) is breaking aliasing rules).  I'll fix it by calling 
getnameinfo(3) (I didn't know that function before; thanks for recommending it!).

And yes, it's a good advice to treat them as black boxes; it avoids many accidents.


> 2. When you need to work with a specific socket address type that `getaddrinfo` and `getnameinfo` don't handle, declare a variable of that specific `struct sockaddr_XX` type, and cast its address to `struct sockaddr *` _only_ at the point of call to a function that requires an argument of that type.  For example, AF_LOCAL / struct sockaddr_un usually has to be handled this way.

Yes.

> 3. To call `getpeername`, or `accept` with non-null second argument, use a union containing each of the concrete socket address structs for the address families you care about (normally at least _in and _in6).  Also include `struct sockaddr` so you can write `&addrbuf.sa` instead of `(struct sockaddr *)&addrbuf`.

And yes.

> 
> However, like with the string functions, I would not use the word "deprecated".

While string functions may be used safely, _storage can't possibly be used 
safely (even for the non-UB case, I wouldn't call it safe, as mentioned above).

Adding a warning in the source code through an attribute may be too aggressive, 
at at this point where POSIX supports it.  But adding some deprecation signs 
(reasonably justified) in the manual so that readers consider stop using it 
might be reasonable, given that it's so buggy (by today's C standards).

I suggest that anyone involved in POSIX reading this considers suggesting an 
official deprecation for the upcoming version.

> 
> zw
> 
> p.s. see https://stackoverflow.com/a/42190913 for my thoughts on the socket address API in way more detail; feel free to crib from there for the manpages

Thanks!  (and nice update :)

Cheers,

Alex

-- 
<http://www.alejandro-colomar.es/>

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]