From:   Alejandro Colomar <alx.manpages@gmail.com>
To:     libc-alpha@sourceware.org
Cc:     Alejandro Colomar <alx@kernel.org>, linux-man@vger.kernel.org,
        =?UTF-8?q?Bastien=20Roucari=C3=A8s?= <rouca@debian.org>,
        Eric Blake <eblake@redhat.com>,
        Zack Weinberg <zack@owlfolio.org>,
        Stefan Puiu <stefan.puiu@gmail.com>,
        Igor Sysoev <igor@sysoev.ru>
Subject: [PATCH] socket: Implement sockaddr_storage with an anonymous union
Date:   Fri, 20 Jan 2023 14:36:28 +0100
Message-Id: <20230120133628.10070-1-alx@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

The historical design of `sockaddr_storage` makes it impossible to use
without breaking strict aliasing rules.  Not only this type is unusable,
but even the use of other `sockaddr_*` structures directly (when the
programmer only cares about a single address family) is also incorrect,
since at some point the structure will be accessed as a `sockaddr`, and
that breaks strict aliasing rules too.

So, the only way for a programmer to not invoke Undefined Behavior is to
declare a union that includes `sockaddr` and any `sockaddr_*` structures
that are of interest, which allows later accessing as either the correct
structure or plain `sockaddr` for the sa_family.

This patch fixes sockaddr_storage to remove UB on its uses and make it
that structure that everybody should be using.  It also allows removing
many casts in code that needs to pass a sockaddr as a side effect.

The following is an example of how this improves both existing code and
new code:

void
foo(foo)
{
    struct old_sockaddr_storage  oss;
    struct new_sockaddr_storage  nss;

    // ... (initialize oss and nss)

    inet_sockaddr2str(&nss.sa);  // correct (and has no casts)
    inet_sockaddr2str((struct sockaddr_storage *)&oss);  // UB
    inet_sockaddr2str((struct sockaddr_storage *)&nss);  // correct
}

/* This function is correct, as far as the accessed object has the
 * type we're using.  That's only possible through a `union`, since
 * we're accessing it with 2 different types: `sockaddr` for the
 * `sa_family` and then the appropriate subtype for the address
 * itself.
 */
const char *
inet_sockaddr2str(const struct sockaddr *sa)
{
    struct sockaddr_in   *sin;
    struct sockaddr_in6  *sin6;

    static char          buf[MAXHOSTNAMELEN];

    switch (sa->sa_family) {
    case AF_INET:
        sin = (struct sockaddr_in *) sa;
        inet_ntop(AF_INET, &sin->sin_addr, buf, NITEMS(buf));
        return buf;
    case AF_INET6:
        sin6 = (struct sockaddr_in6 *) sa;
        inet_ntop(AF_INET6, &sin6->sin6_addr, buf, NITEMS(buf));
        return buf;
    default:
        errno = EAFNOSUPPORT;
        return NULL;
    }
}

While it's not necessary to do the same for `sockaddr`, it might still
be interesting to so, since it will allow removing many casts in the
implementation of many libc functions.

Link: <https://lore.kernel.org/linux-man/2860541.uBSZ6KuyZf@portable-bastien/T/>
Link: <https://inbox.sourceware.org/libc-alpha/0f25d60f-f183-b518-b6c1-6d46aa63ee57@gmail.com/T/>
Link: <https://stackoverflow.com/a/42190913/6872717>
Link: <https://software.codidact.com/posts/287748>
Cc: Bastien Roucariès <rouca@debian.org>
Cc: Eric Blake <eblake@redhat.com>
Cc: Zack Weinberg <zack@owlfolio.org>
Cc: Stefan Puiu <stefan.puiu@gmail.com>
Cc: Igor Sysoev <igor@sysoev.ru>
Signed-off-by: Alejandro Colomar <alx@kernel.org>
---
 bits/socket.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

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.  */
+    union
+      {
+        __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
+        struct sockaddr      sa;
+        struct sockaddr_in   sin;
+        struct sockaddr_in6  sin6;
+        struct sockaddr_un   sun;
+      };
   };
 
 
-- 
2.39.0