From: Alejandro Colomar <alx.manpages@gmail.com>
To: "Bastien Roucariès" <rouca@debian.org>, libc-alpha@sourceware.org
Cc: Alejandro Colomar <alx@kernel.org>,
linux-man@vger.kernel.org, Eric Blake <eblake@redhat.com>,
Zack Weinberg <zack@owlfolio.org>,
Stefan Puiu <stefan.puiu@gmail.com>, Igor Sysoev <igor@sysoev.ru>,
gcc@gcc.gnu.org, Joseph Myers <joseph@codesourcery.com>
Subject: Re: [PATCH v2] socket: Implement sockaddr_storage with an anonymous union
Date: Fri, 20 Jan 2023 21:51:59 +0100 [thread overview]
Message-ID: <4083aa17-dc23-39e3-3a1c-24f751998c75@gmail.com> (raw)
In-Reply-To: <2885282.DR2gb7e7pQ@portable-bastien>
[-- Attachment #1.1: Type: text/plain, Size: 2801 bytes --]
On 1/20/23 21:46, Bastien Roucariès wrote:
> Le vendredi 20 janvier 2023, 20:38:32 UTC Alejandro Colomar a écrit :
>> Hi Bastien,
>>
>> On 1/20/23 21:32, Bastien Roucariès wrote:
>> [...]
>>
>>>> diff --git a/bits/socket.h b/bits/socket.h
>>>> index aac8c49b00..c0c23b4e84 100644
>>>> --- a/bits/socket.h
>>>> +++ b/bits/socket.h
>>>> @@ -168,9 +168,14 @@ struct sockaddr
>>>>
>>>> struct sockaddr_storage
>>>> {
>>>> - __SOCKADDR_COMMON (ss_); /* Address family, etc. */
>>>> - char __ss_padding[_SS_PADSIZE];
>>>> - __ss_aligntype __ss_align; /* Force desired alignment. */
>>> no this is not correct you break ABI by reducing size
>>>> + union
>>>> + {
>>>> + __SOCKADDR_COMMON (ss_); /* Address family, etc. */
>>>> + struct sockaddr sa;
>>>> + struct sockaddr_in sin;
>>>> + struct sockaddr_in6 sin6;
>>>> + struct sockaddr_un sun;
>>>> + };
>>>> };
>>>
>>> Correct one structure is
>>>
>>> struct __private_sock_storage {
>>> __SOCKADDR_COMMON (ssprivate_); /* Address family, etc. */
>>> char __ss_padding[_SS_PADSIZE];
>>> __ss_aligntype __ss_align; /* Force desired alignment. */
>>> }
>>
>> What is this structure for? I expect that it's for declaring a wide-enough and
>> correctly aligned type, but the union containing all the other types already
>> guarantees a size as wide as any other sockaddr_* and with the widest alignment.
>>
>> Also, any member that is necessary for superalignment or padding could be added
>> at the end of sockaddr_storage, after the anon union; you don't need the extra
>> struct, I guess.
>
> No we need it, max of structure is struct sockaddr_un sun and is size of 108.
> sizeof(sockaddr_storage) is 128...
>
> Did you see the line of the kernel source I send you ? kernel expect size of 109 for un aka we should pad by a nul byte...
Yes, I saw it. But that line from the kernel is already Undefined Behavior.
The correct fix should go to sockaddr_un, not sockaddr_storage, IMO.
However, applying this change to sockaddr_storage would expose that kernel bug,
so I think a prepatch to sockaddr_un that adds a padding byte to sockaddr_un
would make sense.
struct sockaddr_un {
__kernel_sa_family_t sun_family; /* AF_UNIX */
char sun_path[UNIX_PATH_MAX]; /* pathname */
char __null; // make sure sun_path is terminated
};
>
> I think it is safer in a first step, to keep the old structure... Maybe later simplify
>
> Did you also see
> https://github.com/bminor/glibc/blob/master/socket/sys/socket.h#L63
Heh, I didn't see that one :)
I'll take it into account for a revision of the patch.
Cheers,
Alex
--
<http://www.alejandro-colomar.es/>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
prev parent reply other threads:[~2023-01-20 20:52 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-01-20 13:40 [PATCH v2] socket: Implement sockaddr_storage with an anonymous union 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
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 [this message]
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=4083aa17-dc23-39e3-3a1c-24f751998c75@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=joseph@codesourcery.com \
--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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.