Re: [PATCH] sockaddr.3type: BUGS: Document that libc should be fixed using a union

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

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

Hi Rich,

On 2/6/23 00:43, Rich Felker wrote:
> On Sun, Feb 05, 2023 at 04:28:36PM +0100, Alejandro Colomar wrote:
>> As discussed before, and Bastien and I seem to agree, ideally we should
>> define the following types:
>>
>>      struct sockaddr_storage {
>>          union {
>>              struct {
>>                  sa_family_t      ss_family;
>>              };
>>              struct sockaddr_in   sin;
>>              struct sockaddr_in6  sin6;
>>              struct sockaddr_un   sun;
>>              // ...
>>          };
>>      };
> 
> AFAIK this is not permitted because of namespace. sys/socket.h is not
> permitted to expose sockaddr_{in,in6,un}. And if you defined
> differently-tagged structures with the same contents, it would not do
> any good; accessing the members with the wrong-tagged struct type
> would still be UB.

I'm not sure.  ISO C has that restriction that a header can only define what the 
standard says it defines.  However, does POSIX have the same restriction? 
Doesn't POSIX allow including any other POSIX headers (maybe it does, but IIRC 
it doesn't)?  Since <sys/socket.h> is just a POSIX thing, that's the only 
standard we should care about.

> 
> Really, there is no action needed here. Nothing is wrong on libc's
> side. The problem is just that the type is *not useful for anything*
> and should not be used except in the context of sizeof, which is
> purely a documentation issue.
> 
>>      struct [[deprecated]] sockaddr {
>>          sa_family_t              sa_family;
>>      };
>>
>>      union [[gnu::transparent_union]] sockaddr_ptr {
>>          struct sockaddr_storage  *ss;
>>          struct sockaddr          *sa;
>>      };
>>
>> And then we could define APIs like:
>>
>>      int bind(int sockfd, const union sockaddr_ptr *addr, socklen_t len);
> 
> You cannot just change APIs because you wish they were different.

This API is compatible.  In fact, it already is now like that:

alx@debian:/usr/include$ grepc bind
./x86_64-linux-gnu/sys/socket.h:112:
extern int bind (int __fd, __CONST_SOCKADDR_ARG __addr, socklen_t __len)
      __THROW;

alx@debian:/usr/include$ sed -n 83,84p x86_64-linux-gnu/sys/socket.h
typedef union { __SOCKADDR_ALLTYPES
	      } __CONST_SOCKADDR_ARG __attribute__ ((__transparent_union__));


> Ideally bind, etc. would just take void *,

void * is a bit too much unsafe.  GCC's transparent unions are a restricted 
catch-many pointer, rather than a catch-all.

> which is what the struct
> sockaddr * is being used as.

And in fact, void* wouldn't sole the union problem.

> But they don't, so callers have to cast.

With the current glibc implementation, you don't need to cast, thanks to the 
[[gnu::transparent_union]]:


alx@debian:~/tmp$ cat sock.c
#define _GNU_SOURCE
#include <sys/socket.h>
#include <sys/un.h>

int
bind_un(int sfd, const struct sockaddr_un *addr, socklen_t len)
{
	return bind(sfd, addr, len);
}
alx@debian:~/tmp$ cc -Wall -Wextra sock.c -S
alx@debian:~/tmp$ clang -Wall -Wextra sock.c -S
alx@debian:~/tmp$


> It's ugly but it's really not a big deal. Much less of a big deal than
> breaking the interface because you think it would look prettier if it
> had been done differently.

It's not breaking the interface; not in GNU C.  Current code still falls back to 
the a POSIX-complying UB-invoking interface when you don't ask for _GNU_SOURCE, 
but we can keep that.  I'm only asking that we fix the GNU C version.  Moreover, 
in POSIX-complying code, you can keep the interface pointer, since you'll need 
to cast anyway, but can still make sockaddr_storage be implemented through an 
anonymous union.

Cheers,

Alex

> 
> Rich

-- 
<http://www.alejandro-colomar.es/>
GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5

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