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 sockaddrstruct 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:
OpenPGP_signature
Description: OpenPGP digital signature
