From: Alejandro Colomar <alx.manpages@gmail.com>
To: "Bastien Roucariès" <rouca@debian.org>,
"Zack Weinberg" <zack@owlfolio.org>,
"GNU libc development" <libc-alpha@sourceware.org>,
gcc@gcc.gnu.org
Cc: Alejandro Colomar <alx@kernel.org>,
'linux-man' <linux-man@vger.kernel.org>,
Eric Blake <eblake@redhat.com>,
Stefan Puiu <stefan.puiu@gmail.com>, Igor Sysoev <igor@sysoev.ru>
Subject: Re: [PATCH v2] socket: Implement sockaddr_storage with an anonymous union
Date: Sat, 21 Jan 2023 15:30:11 +0100 [thread overview]
Message-ID: <c43e0f50-4988-2068-3607-8190747b540f@gmail.com> (raw)
In-Reply-To: <2611573.oxQNTHoq5J@portable-bastien>
[-- Attachment #1.1: Type: text/plain, Size: 4244 bytes --]
Hi Bastien,
On 1/21/23 14:30, Bastien Roucariès wrote:
[...]
>> Ahh, indeed it seems to be UB. It's in the same 6.5.2.3/6: there's a
>> requirement that the information about the union is kept in the function in
>> which it's accessed.
>>
>> The standard presents an example, which is a bit ambiguous:
>>
>> The following is not a valid fragment (because the union type is not
>> visible within function f):
>>
[...]
>>
>> I don't know what's the intention if the union type was global but the variable
>> `u` was still not seen by f(). But it seems GCC's interpretation is UB,
>> according to the test we just saw.
>>
>> The solution that I can see for that is making sockaddr also be a union. That
>> way, the union is kept across all calls (since they all use sockaddr).
>>
>> struct sockaddr {
>> union {
>> struct {
>> sa_family_t sa_family;
>> char sa_data[14]; // why 14?
>> }
>> struct sockaddr_in sin;
>> struct sockaddr_in6 sin6;
>> struct sockaddr_un sun;
>> };
>> };
>
> No the solution is to avoid sockaddr and mark as deprecated.
Declaring `sockaddr` as deprecated means deprecating also:
accept(2)
accept4(2)
bind(2)
connect(2)
getpeername(2)
getsockname(2)
recvfrom(2)
sendto(2)
getnameinfo(3)
which use the type in their prototype.
Also, other types such as `addrinfo`, which contain `sockaddr` would also need
to be deprecated, which would itself require deprecating:
getaddrinfo(3)
freeaddrinfo(3)
Since addrinfo is itself contained in other structures such as `gaicb`, we would
also need to deprecate those, which would in turn require deprecating more APIs:
getaddrinfo_a(3)
gai_error(3)
gai_cancel(3)
And maybe I left some. This feels like nuking the entire networking API, which
I don't see happening soon.
Otherwise, we need to come up with a solution that keeps these APIs compatible
with whatever new type we suggest using. Changing them to use `void*` instead
of `sockaddr*` would be possible, but would decrease type safety considerably,
so there should be a good reason for that.
Suggesting to use always `sockaddr_storage` but using `sockaddr` in the function
parameters keeps the current not-so-nice casting issues, which are not Undefined
Behavior per se, but not ideal either (in fact, I don't think `void*` is much
worse than code full of casts). And it would also be error-prone, since users
could get the idea that `sockaddr` can be used safely, since it's what gets
passed as the parameter.
> The problem it should be part of union without raising a warning each time we use a safe type...
I don't understand this; please detail.
>
> The other solution is to render public and ABI stable the type here
> https://github.com/bminor/glibc/blob/ae612c45efb5e34713859a5facf92368307efb6e/socket/sys/socket.h#L78
> under for instance sockaddr_ptr and sockaddr_const_ptr
[[gnu::transparent_union]] (used in that link) seems like a "the design of this
interface is bad, sorry, this workaround will just make it work". I guess it
just works, and probably it's the reason that so much undefined behavior hasn't
exploded so far. However, if we can solve this using core language features,
I'd go that way.
>
> Moreover this are some patch arch by arch
> https://sourceware.org/legacy-ml/libc-alpha/2016-02/msg00340.html that should be made default
>
> Bastien
>
>
>
>>
>> struct sockaddr_storage {
>> union {
>> sa_family_t ss_family;
>> struct sockaddr sa;
>> };
>> };
Hmm, this isn't still perfect. Since the APIs get the sockaddr, this union
information would be lost. `sockaddr` needs to be the type that is declared.
`sockaddr_storage` should just die; there's no way to make it compatible with
APIs getting a `sockaddr`. The attribute `transparent_union` is the only way to
use is safely.
Cheers,
Alex
>>
>>
>> With this, sockaddr_storage becomes useless, but still usable. New code, could
>> just use sockaddr and use the internal union members as necessary. Union info
>> is kept across all function boundaries.
--
<http://www.alejandro-colomar.es/>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
next prev parent reply other threads:[~2023-01-21 14:30 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-20 13:40 Alejandro Colomar
2023-01-20 17:49 ` Joseph Myers
2023-01-20 19:26 ` Alejandro Colomar
2023-01-20 18:04 ` Zack Weinberg
2023-01-20 19:25 ` Alejandro Colomar
2023-01-21 2:38 ` Alejandro Colomar
2023-01-21 3:17 ` Alejandro Colomar
2023-01-21 13:30 ` Bastien Roucariès
2023-01-21 14:30 ` Alejandro Colomar [this message]
2023-01-22 14:12 ` Bastien Roucariès
2023-01-20 20:32 ` Bastien Roucariès
2023-01-20 20:38 ` Alejandro Colomar
2023-01-20 20:46 ` Bastien Roucariès
2023-01-20 20:51 ` Alejandro Colomar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c43e0f50-4988-2068-3607-8190747b540f@gmail.com \
--to=alx.manpages@gmail.com \
--cc=alx@kernel.org \
--cc=eblake@redhat.com \
--cc=gcc@gcc.gnu.org \
--cc=igor@sysoev.ru \
--cc=libc-alpha@sourceware.org \
--cc=linux-man@vger.kernel.org \
--cc=rouca@debian.org \
--cc=stefan.puiu@gmail.com \
--cc=zack@owlfolio.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).