Don't deference type-punned pointers

Dereferencing type-punned pointers is undefined behaviour according to the
C standard.  We should either obey the standard, or ensure that all
supported compilers deal with dereferencing type-punned pointers as we
want them to.  I think just obeying the standard is the easiest solution.

See e.g. http://blog.regehr.org/archives/959.

This commit refactors the offending code to use unions or memcpy() to
comply to strict aliasing rules.

Note that this also slightly changes mroute_addr_mask_host_bits(), to
behave as it was probably intended to:  only mask the address part, not
also the port part of IPv6 adresses if MR_WITH_PORT is used (ie ma->len
is sizeof(struct in6_addr)+2).

v2: fix all strict aliasing occurrences, not just those in mroute.h
v3: add missing ntohs() in mroute_addr_print_ex()

Signed-off-by: Steffan Karger <steffan@karger.me>
Acked-by: Gert Doering <gert@greenie.muc.de>
Message-Id: <1479043047-25883-1-git-send-email-steffan@karger.me>
URL: https://www.mail-archive.com/openvpn-devel@lists.sourceforge.net/msg13017.html
Signed-off-by: Gert Doering <gert@greenie.muc.de>

diff --git a/src/openvpn/error.h b/src/openvpn/error.h
index ced2fdfff9c01fcb02282562801a5e165bbeb434..c90b36be25e29abb1ddf3aa8857a5ea97a3d3874 100644 (file)
--- a/src/openvpn/error.h
+++ b/src/openvpn/error.h
@@ -27,6 +27,8 @@
 
 #include "basic.h"
 
+#include <assert.h>
+
 /* #define ABORT_ON_ERROR */
 
 #ifdef ENABLE_PKCS11
@@ -219,6 +221,14 @@ FILE *msg_fp(const unsigned int flags);
 void assert_failed (const char *filename, int line, const char *condition)
   __attribute__((__noreturn__));
 
+/* Poor-man's static_assert() for when not supplied by assert.h, taken from
+ * Linux's sys/cdefs.h under GPLv2 */
+#ifndef static_assert
+#define static_assert(expr, diagnostic) \
+    extern int (*__OpenVPN_static_assert_function (void)) \
+      [!!sizeof (struct { int __error_if_negative: (expr) ? 2 : -1; })]
+#endif
+
 #ifdef ENABLE_DEBUG
 void crash (void); /* force a segfault (debugging only) */
 #endif

diff --git a/src/openvpn/mroute.c b/src/openvpn/mroute.c
index 972f1dd5e454a360a44b7066ac1e0d1ee57e4ef5..c905af78626c340b0c57375d783be5f7892f6e8d 100644 (file)
--- a/src/openvpn/mroute.c
+++ b/src/openvpn/mroute.c
@@ -58,7 +58,8 @@ is_mac_mcast_addr (const uint8_t *mac)
 static inline bool
 is_mac_mcast_maddr (const struct mroute_addr *addr)
 {
-  return (addr->type & MR_ADDR_MASK) == MR_ADDR_ETHER && is_mac_mcast_addr (addr->addr); 
+  return (addr->type & MR_ADDR_MASK) == MR_ADDR_ETHER &&
+      is_mac_mcast_addr (addr->eth_addr);
 }
 
 /*
@@ -73,7 +74,7 @@ mroute_learnable_address (const struct mroute_addr *addr)
 
   for (i = 0; i < addr->len; ++i)
     {
-      int b = addr->addr[i];
+      int b = addr->raw_addr[i];
       if (b != 0x00)
        not_all_zeros = true;
       if (b != 0xFF)
@@ -90,7 +91,7 @@ mroute_get_in_addr_t (struct mroute_addr *ma, const in_addr_t src, unsigned int
       ma->type = MR_ADDR_IPV4 | mask;
       ma->netbits = 0;
       ma->len = 4;
-      *(in_addr_t*)ma->addr = src;
+      ma->v4.addr = src;
     }
 }
 
@@ -102,7 +103,7 @@ mroute_get_in6_addr (struct mroute_addr *ma, const struct in6_addr src, unsigned
       ma->type = MR_ADDR_IPV6 | mask;
       ma->netbits = 0;
       ma->len = 16;
-      *(struct in6_addr *)ma->addr = src;
+      ma->v6.addr = src;
     }
 }
 
@@ -226,14 +227,14 @@ mroute_extract_addr_ether (struct mroute_addr *src,
          src->type = MR_ADDR_ETHER;
          src->netbits = 0;
          src->len = 6;
-         memcpy (src->addr, eth->source, 6);
+         memcpy (src->eth_addr, eth->source, sizeof(dest->eth_addr));
        }
       if (dest)
        {
          dest->type = MR_ADDR_ETHER;
          dest->netbits = 0;
          dest->len = 6;
-         memcpy (dest->addr, eth->dest, 6);
+         memcpy (dest->eth_addr, eth->dest, sizeof(dest->eth_addr));
 
          /* ethernet broadcast/multicast packet? */
          if (is_mac_mcast_addr (eth->dest))
@@ -281,15 +282,15 @@ bool mroute_extract_openvpn_sockaddr (struct mroute_addr *addr,
          addr->type = MR_ADDR_IPV4 | MR_WITH_PORT;
          addr->netbits = 0;
          addr->len = 6;
-         memcpy (addr->addr, &osaddr->addr.in4.sin_addr.s_addr, 4);
-         memcpy (addr->addr + 4, &osaddr->addr.in4.sin_port, 2);
+         addr->v4.addr = osaddr->addr.in4.sin_addr.s_addr;
+         addr->v4.port = osaddr->addr.in4.sin_port;
        }
       else
        {
          addr->type = MR_ADDR_IPV4;
          addr->netbits = 0;
          addr->len = 4;
-         memcpy (addr->addr, &osaddr->addr.in4.sin_addr.s_addr, 4);
+         addr->v4.addr = osaddr->addr.in4.sin_addr.s_addr;
        }
       return true;
     }
@@ -299,15 +300,15 @@ bool mroute_extract_openvpn_sockaddr (struct mroute_addr *addr,
          addr->type = MR_ADDR_IPV6 | MR_WITH_PORT;
          addr->netbits = 0;
          addr->len = 18;
-         memcpy (addr->addr, &osaddr->addr.in6.sin6_addr, 16);
-         memcpy (addr->addr + 16, &osaddr->addr.in6.sin6_port, 2);
+         addr->v6.addr = osaddr->addr.in6.sin6_addr;
+         addr->v6.port = osaddr->addr.in6.sin6_port;
        }
       else
        {
          addr->type = MR_ADDR_IPV6;
          addr->netbits = 0;
          addr->len = 16;
-         memcpy (addr->addr, &osaddr->addr.in6.sin6_addr, 16);
+         addr->v6.addr = osaddr->addr.in6.sin6_addr;
        }
       return true;
   }
@@ -326,23 +327,29 @@ bool mroute_extract_openvpn_sockaddr (struct mroute_addr *addr,
 void
 mroute_addr_mask_host_bits (struct mroute_addr *ma)
 {
-  in_addr_t addr = ntohl(*(in_addr_t*)ma->addr);
   if ((ma->type & MR_ADDR_MASK) == MR_ADDR_IPV4)
     {
+      in_addr_t addr = ntohl (ma->v4.addr);
       addr &= netbits_to_netmask (ma->netbits);
-      *(in_addr_t*)ma->addr = htonl (addr);
+      ma->v4.addr = htonl (addr);
     }
   else if ((ma->type & MR_ADDR_MASK) == MR_ADDR_IPV6)
     {
-      int byte = ma->len-1;            /* rightmost byte in address */
+      int byte = sizeof (ma->v6.addr) - 1;     /* rightmost byte in address */
       int bits_to_clear = 128 - ma->netbits;
 
       while( byte >= 0 && bits_to_clear > 0 )
         {
          if ( bits_to_clear >= 8 )
-           { ma->addr[byte--] = 0; bits_to_clear -= 8; }
+           {
+             ma->v6.addr.s6_addr[byte--] = 0;
+             bits_to_clear -= 8;
+           }
          else
-           { ma->addr[byte--] &= (IPV4_NETMASK_HOST << bits_to_clear); bits_to_clear = 0; }
+           {
+             ma->v6.addr.s6_addr[byte--] &= (IPV4_NETMASK_HOST << bits_to_clear);
+             bits_to_clear = 0;
+           }
         }
       ASSERT( bits_to_clear == 0 );
     }
@@ -390,51 +397,41 @@ mroute_addr_print_ex (const struct mroute_addr *ma,
       switch (maddr.type & MR_ADDR_MASK)
        {
        case MR_ADDR_ETHER:
-         buf_printf (&out, "%s", format_hex_ex (ma->addr, 6, 0, 1, ":", gc)); 
+         buf_printf (&out, "%s", format_hex_ex (ma->eth_addr,
+             sizeof(ma->eth_addr), 0, 1, ":", gc));
          break;
        case MR_ADDR_IPV4:
          {
-           struct buffer buf;
-           in_addr_t addr;
-           int port;
-           bool status;
-           buf_set_read (&buf, maddr.addr, maddr.len);
-           addr = buf_read_u32 (&buf, &status);
-           if (status)
+           if ((flags & MAPF_SHOW_ARP) && (maddr.type & MR_ARP))
+             buf_printf (&out, "ARP/");
+           buf_printf (&out, "%s", print_in_addr_t (ntohl (maddr.v4.addr),
+               (flags & MAPF_IA_EMPTY_IF_UNDEF) ? IA_EMPTY_IF_UNDEF : 0, gc));
+           if (maddr.type & MR_WITH_NETBITS)
              {
-               if ((flags & MAPF_SHOW_ARP) && (maddr.type & MR_ARP))
-                 buf_printf (&out, "ARP/");
-               buf_printf (&out, "%s", print_in_addr_t (addr, (flags & MAPF_IA_EMPTY_IF_UNDEF) ? IA_EMPTY_IF_UNDEF : 0, gc));
-               if (maddr.type & MR_WITH_NETBITS)
+               if (flags & MAPF_SUBNET)
                  {
-                   if (flags & MAPF_SUBNET)
-                     {
-                       const in_addr_t netmask = netbits_to_netmask (maddr.netbits);
-                       buf_printf (&out, "/%s", print_in_addr_t (netmask, 0, gc));
-                     }
-                   else
-                     buf_printf (&out, "/%d", maddr.netbits);
+                   const in_addr_t netmask = netbits_to_netmask (maddr.netbits);
+                   buf_printf (&out, "/%s", print_in_addr_t (netmask, 0, gc));
                  }
+               else
+                 buf_printf (&out, "/%d", maddr.netbits);
              }
            if (maddr.type & MR_WITH_PORT)
              {
-               port = buf_read_u16 (&buf);
-               if (port >= 0)
-                 buf_printf (&out, ":%d", port);
+               buf_printf (&out, ":%d", ntohs (maddr.v4.port));
              }
          }
          break;
        case MR_ADDR_IPV6:
          {
-           if ( IN6_IS_ADDR_V4MAPPED( (struct in6_addr*)&maddr.addr ) )
+           if ( IN6_IS_ADDR_V4MAPPED( &maddr.v6.addr ) )
              {
-               buf_printf (&out, "%s",
-                    print_in_addr_t( *(in_addr_t*)(&maddr.addr[12]), IA_NET_ORDER, gc));
+               buf_printf (&out, "%s", print_in_addr_t (maddr.v4mappedv6.addr,
+                   IA_NET_ORDER, gc));
              }
            else
              {
-               buf_printf (&out, "%s",
-                     print_in6_addr( *(struct in6_addr*)&maddr.addr, 0, gc));
+               buf_printf (&out, "%s", print_in6_addr (maddr.v6.addr, 0, gc));
              }
            if (maddr.type & MR_WITH_NETBITS)
              {

diff --git a/src/openvpn/mroute.h b/src/openvpn/mroute.h
index 608f70bef2a9a9fa07bf4e956d8a4ce14e6c5145..5fe17e7f31e9f2e777873d095fae3ed1344b5178 100644 (file)
--- a/src/openvpn/mroute.h
+++ b/src/openvpn/mroute.h
@@ -31,6 +31,8 @@
 #include "list.h"
 #include "route.h"
 
+#include <stddef.h>
+
 #define IP_MCAST_SUBNET_MASK  ((in_addr_t)240<<24)
 #define IP_MCAST_NETWORK      ((in_addr_t)224<<24)
 
@@ -79,9 +81,35 @@ struct mroute_addr {
   uint8_t type;     /* MR_ADDR/MR_WITH flags */
   uint8_t netbits;  /* number of bits in network part of address,
                       valid if MR_WITH_NETBITS is set */
-  uint8_t addr[MR_MAX_ADDR_LEN];  /* actual address */
+  union {
+    uint8_t raw_addr[MR_MAX_ADDR_LEN];  /* actual address */
+    uint8_t eth_addr[OPENVPN_ETH_ALEN];
+    struct {
+      in_addr_t addr;          /* _network order_ IPv4 address */
+      in_port_t port;          /* _network order_ TCP/UDP port */
+    } v4;
+    struct {
+      struct in6_addr addr;
+      in_port_t port;          /* _network order_ TCP/UDP port */
+    } v6;
+    struct {
+      uint8_t prefix[12];
+      in_addr_t addr;          /* _network order_ IPv4 address */
+    } v4mappedv6;
+  };
 };
 
+/* Double-check that struct packing works as expected */
+static_assert (offsetof(struct mroute_addr, v4.port) ==
+       offsetof(struct mroute_addr, v4) + 4,
+    "Unexpected struct packing of v4");
+static_assert (offsetof(struct mroute_addr, v6.port) ==
+       offsetof(struct mroute_addr, v6) + 16,
+    "Unexpected struct packing of v6");
+static_assert (offsetof(struct mroute_addr, v4mappedv6.addr) ==
+       offsetof(struct mroute_addr, v4mappedv6) + 12,
+    "Unexpected struct packing of v4mappedv6");
+
 /*
  * Number of bits in an address.  Should be raised for IPv6.
  */
@@ -167,7 +195,7 @@ mroute_addr_equal (const struct mroute_addr *a1, const struct mroute_addr *a2)
     return false;
   if (a1->len != a2->len)
     return false;
-  return memcmp (a1->addr, a2->addr, a1->len) == 0;
+  return memcmp (a1->raw_addr, a2->raw_addr, a1->len) == 0;
 }
 
 static inline const uint8_t *
@@ -189,16 +217,17 @@ mroute_extract_in_addr_t (struct mroute_addr *dest, const in_addr_t src)
   dest->type = MR_ADDR_IPV4;
   dest->netbits = 0;
   dest->len = 4;
-  *(in_addr_t*)dest->addr = htonl (src);
+  dest->v4.addr = htonl (src);
 }
 
 static inline in_addr_t
 in_addr_t_from_mroute_addr (const struct mroute_addr *addr)
 {
-  if ((addr->type & MR_ADDR_MASK) == MR_ADDR_IPV4 && addr->netbits == 0 && addr->len == 4)
-    return ntohl(*(in_addr_t*)addr->addr);
-  else
+  if ((addr->type & MR_ADDR_MASK) == MR_ADDR_IPV4 && addr->netbits == 0 && addr->len == 4) {
+    return ntohl(addr->v4.addr);
+  } else {
     return 0;
+  }
 }
 
 static inline void

diff --git a/src/openvpn/multi.c b/src/openvpn/multi.c
index 93a554d0a37e499019bdf1f01a49794a238de01a..dc4cff47e07eea9114ff883764ed2f0c65c84ca7 100644 (file)
--- a/src/openvpn/multi.c
+++ b/src/openvpn/multi.c
@@ -1201,7 +1201,7 @@ multi_learn_in6_addr  (struct multi_context *m,
   addr.len = 16;
   addr.type = MR_ADDR_IPV6;
   addr.netbits = 0;
-  memcpy( &addr.addr, &a6, sizeof(a6) );
+  addr.v6.addr = a6;
 
   if (netbits >= 0)
     {
@@ -2435,7 +2435,7 @@ multi_process_incoming_link (struct multi_context *m, struct multi_instance *ins
                {
                  /* IPv6 link-local address (fe80::xxx)? */
                  if ( (src.type & MR_ADDR_MASK) == MR_ADDR_IPV6 &&
-                       src.addr[0] == 0xfe && src.addr[1] == 0x80 )
+                       IN6_IS_ADDR_LINKLOCAL (&src.v6.addr) )
                    {
                      /* do nothing, for now.  TODO: add address learning */
                    }

diff --git a/src/openvpn/ps.c b/src/openvpn/ps.c
index fe18a9df53566722efc74118d167a9b601d44c52..2cb68f170a5b0664b2714dd364f1b462c72c0897 100644 (file)
--- a/src/openvpn/ps.c
+++ b/src/openvpn/ps.c
@@ -223,12 +223,12 @@ port_share_sendmsg (const socket_descriptor_t sd,
 
       if (socket_defined (sd_send))
        {
-         *((socket_descriptor_t*)CMSG_DATA(h)) = sd_send;
+         memcpy (CMSG_DATA(h), &sd_send, sizeof (sd_send));
        }
       else
        {
          socketpair (PF_UNIX, SOCK_DGRAM, 0, sd_null);
-         *((socket_descriptor_t*)CMSG_DATA(h)) = sd_null[0];
+         memcpy (CMSG_DATA(h), &sd_null[0], sizeof (sd_null[0]));
        }
 
       status = sendmsg (sd, &mesg, MSG_NOSIGNAL);
@@ -502,7 +502,8 @@ control_message_from_parent (const socket_descriptor_t sd_control,
   h->cmsg_len = CMSG_LEN(sizeof(socket_descriptor_t));
   h->cmsg_level = SOL_SOCKET;
   h->cmsg_type = SCM_RIGHTS;
-  *((socket_descriptor_t*)CMSG_DATA(h)) = SOCKET_UNDEFINED;
+  static const socket_descriptor_t socket_undefined = SOCKET_UNDEFINED;
+  memcpy (CMSG_DATA(h), &socket_undefined, sizeof(socket_undefined));
 
   status = recvmsg (sd_control, &mesg, MSG_NOSIGNAL);
   if (status != -1)
@@ -516,7 +517,8 @@ control_message_from_parent (const socket_descriptor_t sd_control,
        }
       else
        {
-         const socket_descriptor_t received_fd = *((socket_descriptor_t*)CMSG_DATA(h));
+         socket_descriptor_t received_fd;
+         memcpy (&received_fd, CMSG_DATA(h), sizeof(received_fd));
          dmsg (D_PS_PROXY_DEBUG, "PORT SHARE PROXY: RECEIVED sd=%d", (int)received_fd);
 
          if (status >= 2 && command == COMMAND_REDIRECT)

diff --git a/src/openvpn/socket.c b/src/openvpn/socket.c
index 18e64097386ce9d3f98e9d22ec352604fbe1e562..1fbb415300a9b28e428e8e6777d3c5a558ffa595 100644 (file)
--- a/src/openvpn/socket.c
+++ b/src/openvpn/socket.c
@@ -2598,7 +2598,8 @@ setenv_sockaddr (struct env_set *es, const char *name_prefix, const struct openv
       if ( IN6_IS_ADDR_V4MAPPED( &addr->addr.in6.sin6_addr ))
        {
          struct in_addr ia;
-         ia.s_addr = *(in_addr_t *)&addr->addr.in6.sin6_addr.s6_addr[12] ;
+         memcpy (&ia.s_addr, &addr->addr.in6.sin6_addr.s6_addr[12],
+             sizeof (ia.s_addr));
          openvpn_snprintf (name_buf, sizeof (name_buf), "%s_ip", name_prefix);
          openvpn_snprintf (buf, sizeof(buf), "%s", inet_ntoa(ia) );
        }