[CC += Zack, glibc] Hi Bastien, Eric, and Zack, On 1/19/23 20:44, Bastien Roucariès wrote:
Le jeudi 19 janvier 2023, 12:42:52 UTC Alejandro Colomar a écrit : Hi all, [adding Eric Blake from redhat and if I remember well member of POSIX comitee]
Thanks.BTW, your post triggered some changes. I didn't CC to not be too noisy, but since you seem to be interested in it, here are a few things:
- Suggest deprecating struct sockaddr_storage to glibc: <https://inbox.sourceware.org/libc-alpha/0f25d60f-f183-b518-b6c1-6d46aa63ee57@xxxxxxxxx/T/> - Which caused Zack to update some old stackoverflow answer :) <https://stackoverflow.com/a/42190913/6872717>- He made me realize getnameinfo(3) exists, and so I used in place of a similar function that I had implemented recently:
<https://github.com/shadow-maint/shadow/pull/629>- And while reading the manual page for getnameinfo(3), I realized it was missing some missing _Nullable qualifiers:
<https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/commit/?id=39017f90644141a8bd297c783201e04b238769fb>BTW, Zack, glibc could add [[nonnull(1)]] for the prototype. Should I send a patch adding it to the header?
Eric to be short posix behavior of sockaddr sockaddr_storage is UB
Agree. Almost all uses of struct sockaddr_storage are UB, and those that are not UB, are error prone. stockaddr_storage is bad.
and example supplied are UB. See here: https://pubs.opengroup.org/onlinepubs/009696699/basedefs/sys/socket.h.html
I don't see any usage examples there. Only a definition. The definition isn't bad in itself; it's just that it's unusable.
BTW, now we need to check all the man pages that refer to that structure and fix them to not promote UB.
I think we could save the day by defining struct sockadd_storage using anonymous union (thanks to C11 support), but it will leave C++. But how can be done...Hi Bastien, Hi Bastien, On 1/17/23 11:22, Bastien Roucariès wrote:Hi, I have a lot of student that does not use correctly getsockname in case of dual stack.Please show some examples of common mistakes, if you can.oh the basic one is to pass a sockaddr_in instead of sockaddr_in6... Or to pass a sockaddr_in6 and do not think that it could be returned an IPV4 socket or an Unix stream socket.
Understood.
May be this kind of discussion should be factorized in sockaddr_storage (the historical note particularly). i suppose the same should be done for getpeername I think a safe programming example may be given that accept a socket as stdin and print information on it. Using socat it could be simple to test.Please provide some if you can. However, be careful, since it might easily fall into Undefined Behavior.Ok see it attached.
Thanks.
maybe forcing ENOTSUPP if *addr > sizeof(sockadd_storage) Regards BastienI'll add some comments to the patch.From 0afb3ad23f8ea09331f21a377e3ad19c44e4df18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bastien=20Roucari=C3=A8s?= <rouca@xxxxxxxxxx> Date: Tue, 17 Jan 2023 10:07:43 +0000 Subject: [PATCH] Document use of struct sockaddr_storage in getsockname Document: - storage requierement - future compatibility --- man2/getsockname.2 | 56 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/man2/getsockname.2 b/man2/getsockname.2 index e6e8980c9..5914c9e12 100644 --- a/man2/getsockname.2 +++ b/man2/getsockname.2 @@ -39,6 +39,17 @@ The returned address is truncated if the buffer provided is too small; in this case, .I addrlen will return a value greater than was supplied to the call. +.PP +For greater portability +.I addr +should point to a structure of type +.I sockaddr_storage.This is not correct. If the object has a type of 'struct sockaddr_storage', then it can only be accessed as a 'struct sockaddr_storage'. Trying to access it as any other structure type would be Undefined Behavior. The way to do it conforming to ISO C would be to declare a union which contains all the 'struct sockaddr_*' that are interesting, and access it through the correct union member. Anything else is on UB land. The only use of 'struct sockaddr_storage' that I can think of, is for having a more consistent union size.Ok I see, Eric could we redefine at posix level the struct sockaddr_storage to be something like, it seems that it is allowed by /* * Following fields are implementation-defined. */ union sockaddr_storage { sa_family_t ss_family; struct sockaddr sock; struct _sockaddr_storage storage; struct sockaddr_in in; struct sockaddr_in6 in6; struct sockaddr_un un; };
Hmmm, interesting. We can't do that, because this would be a breaking change to source code. We can't change from `struct sockaddr_storage` to `union sockaddr_storage`. However, this suggestion, combined with the idea of using an anonymous union, can be made to work as something that is compatible with the current sockaddr_storage:
struct sockaddr_storage {
union {
sa_family_t ss_family;
struct sockaddr sa;
struct sockaddr_in sin;
struct sockaddr_in6 sin6;
struct sockaddr_un sun;
};
};
This is compatible:
- It had at least the `ss_family` field. It's still there, at the same binary
location.
- It has a size at least as large as any other sockaddr_* structure, and a
suitable alignment.
- Old code still works with it just fine.- New code will be able to avoid UB, and all casts, just by accessing the right structure element. - It's trivial to test at configure time if the implementation provides this new definition of the structure.
+.I sockaddr_storage +structure is large enough to hold any of the other +.I sockaddr_* +variants and always well aligned. On return, it should be cast to the correct +.I sockaddr_*The fact that it is correctly aligned, and a cast will work most of the time, isn't enough for strict aliasing rules. The compiler is free to assume things, just by the fact that it's a different type.Ok any idea for writing this kind of stuff
I'm thinking about writing something to several pages; will keep you all updated on important changes to the pages.
Cheers, Alex -- <http://www.alejandro-colomar.es/>
Attachment:
OpenPGP_signature
Description: OpenPGP digital signature
