Re: [PATCH] Fix use of sockaddr_in

[Paul Eggert <eggert@cs.ucla.edu>]

[-- Attachment #1: Type: text/plain, Size: 989 bytes --]

On 05/15/2017 02:04 AM, Philipp Stephani wrote:
> - Maybe add verify(INT_MAX >= TYPE_MAXIMUM(in_port_t)) and 
> verify(TYPE_MINIMUM(in_port_t) == 0) to make sure that we can use int 
> for the port?

The code has been changed so that the port number is no longer stored in 
an int so this remark is obsolete now. However, if this issue crops up 
later I don't think we need to worry about it for IPv4 and IPv6 ports, 
since they're always in the range 0..65535. Emacs already assumes that 
'int' is at least 32 bits wide, as per POSIX.

> - Should there be a guard against the alias violation, e.g. by 
> declaring sa and sa1 with __attribute__((may_alias))? Otherwise it's 
> UB and the compiler might elide the switch entirely.

Yes, that is easy enough to do and would avoid some unlikely but 
hard-to-catch bugs. I installed the attached. Most likely other parts of 
Emacs should use may_alias too; do you happen to have any tool in your 
toolbox that would find them systematically?


[-- Attachment #2: 0001-Avoid-undefined-behavior-in-struct-sockaddr.patch --]
[-- Type: text/x-patch, Size: 6297 bytes --]

From 2c70724a4d9809d34caefa0903012d8449b62a3c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Wed, 17 May 2017 13:35:52 -0700
Subject: [PATCH] Avoid undefined behavior in struct sockaddr

Problem noted by Philipp Stephani in:
http://lists.gnu.org/archive/html/emacs-devel/2017-05/msg00391.html
* src/conf_post.h (ATTRIBUTE_MAY_ALIAS, DECLARE_POINTER_ALIAS):
New macros.
* src/process.c (conv_sockaddr_to_lisp, conv_lisp_to_sockaddr)
(connect_network_socket, network_interface_info)
(server_accept_connection): Use it when aliasing non-char objects.
---
 src/conf_post.h | 14 ++++++++++++++
 src/process.c   | 31 +++++++++++++++++--------------
 2 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/src/conf_post.h b/src/conf_post.h
index 1462bd1..c05c93b 100644
--- a/src/conf_post.h
+++ b/src/conf_post.h
@@ -263,6 +263,20 @@ extern int emacs_setenv_TZ (char const *);
 #define ATTRIBUTE_CONST _GL_ATTRIBUTE_CONST
 #define ATTRIBUTE_UNUSED _GL_UNUSED
 
+#if GNUC_PREREQ (3, 3, 0)
+# define ATTRIBUTE_MAY_ALIAS __attribute__ ((__may_alias__))
+#else
+# define ATTRIBUTE_MAY_ALIAS
+#endif
+
+/* Declare NAME to be a pointer to an object of type TYPE, initialized
+   to the address ADDR, which may be of a different type.  Accesses
+   via NAME may alias with other accesses with the traditional
+   behavior, even if options like gcc -fstrict-aliasing are used.  */
+
+#define DECLARE_POINTER_ALIAS(name, type, addr) \
+  type ATTRIBUTE_MAY_ALIAS *name = (type *) (addr)
+
 #if 3 <= __GNUC__
 # define ATTRIBUTE_MALLOC __attribute__ ((__malloc__))
 #else
diff --git a/src/process.c b/src/process.c
index 8180fea..c301739 100644
--- a/src/process.c
+++ b/src/process.c
@@ -2476,7 +2476,7 @@ conv_sockaddr_to_lisp (struct sockaddr *sa, ptrdiff_t len)
     {
     case AF_INET:
       {
-	struct sockaddr_in *sin = (struct sockaddr_in *) sa;
+	DECLARE_POINTER_ALIAS (sin, struct sockaddr_in, sa);
 	len = sizeof (sin->sin_addr) + 1;
 	address = Fmake_vector (make_number (len), Qnil);
 	p = XVECTOR (address);
@@ -2487,8 +2487,8 @@ conv_sockaddr_to_lisp (struct sockaddr *sa, ptrdiff_t len)
 #ifdef AF_INET6
     case AF_INET6:
       {
-	struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *) sa;
-	uint16_t *ip6 = (uint16_t *) &sin6->sin6_addr;
+	DECLARE_POINTER_ALIAS (sin6, struct sockaddr_in6, sa);
+	DECLARE_POINTER_ALIAS (ip6, uint16_t, &sin6->sin6_addr);
 	len = sizeof (sin6->sin6_addr) / 2 + 1;
 	address = Fmake_vector (make_number (len), Qnil);
 	p = XVECTOR (address);
@@ -2501,7 +2501,7 @@ conv_sockaddr_to_lisp (struct sockaddr *sa, ptrdiff_t len)
 #ifdef HAVE_LOCAL_SOCKETS
     case AF_LOCAL:
       {
-	struct sockaddr_un *sockun = (struct sockaddr_un *) sa;
+	DECLARE_POINTER_ALIAS (sockun, struct sockaddr_un, sa);
         ptrdiff_t name_length = len - offsetof (struct sockaddr_un, sun_path);
         /* If the first byte is NUL, the name is a Linux abstract
            socket name, and the name can contain embedded NULs.  If
@@ -2612,7 +2612,7 @@ conv_lisp_to_sockaddr (int family, Lisp_Object address, struct sockaddr *sa, int
       p = XVECTOR (address);
       if (family == AF_INET)
 	{
-	  struct sockaddr_in *sin = (struct sockaddr_in *) sa;
+	  DECLARE_POINTER_ALIAS (sin, struct sockaddr_in, sa);
 	  len = sizeof (sin->sin_addr) + 1;
 	  hostport = XINT (p->contents[--len]);
 	  sin->sin_port = htons (hostport);
@@ -2622,8 +2622,8 @@ conv_lisp_to_sockaddr (int family, Lisp_Object address, struct sockaddr *sa, int
 #ifdef AF_INET6
       else if (family == AF_INET6)
 	{
-	  struct sockaddr_in6 *sin6 = (struct sockaddr_in6 *) sa;
-	  uint16_t *ip6 = (uint16_t *)&sin6->sin6_addr;
+	  DECLARE_POINTER_ALIAS (sin6, struct sockaddr_in6, sa);
+	  DECLARE_POINTER_ALIAS (ip6, uint16_t, &sin6->sin6_addr);
 	  len = sizeof (sin6->sin6_addr) / 2 + 1;
 	  hostport = XINT (p->contents[--len]);
 	  sin6->sin6_port = htons (hostport);
@@ -2645,7 +2645,7 @@ conv_lisp_to_sockaddr (int family, Lisp_Object address, struct sockaddr *sa, int
 #ifdef HAVE_LOCAL_SOCKETS
       if (family == AF_LOCAL)
 	{
-	  struct sockaddr_un *sockun = (struct sockaddr_un *) sa;
+	  DECLARE_POINTER_ALIAS (sockun, struct sockaddr_un, sa);
 	  cp = SDATA (address);
 	  for (i = 0; i < sizeof (sockun->sun_path) && *cp; i++)
 	    sockun->sun_path[i] = *cp++;
@@ -3436,13 +3436,15 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
 		       == offsetof (struct sockaddr_in6, sin6_port))
 		      && sizeof (sa1.sin_port) == sizeof (sa6.sin6_port));
 #endif
-	      if (getsockname (s, (struct sockaddr *)&sa1, &len1) == 0)
+	      DECLARE_POINTER_ALIAS (psa1, struct sockaddr, &sa1);
+	      if (getsockname (s, psa1, &len1) == 0)
 		{
 		  Lisp_Object service = make_number (ntohs (sa1.sin_port));
 		  contact = Fplist_put (contact, QCservice, service);
 		  /* Save the port number so that we can stash it in
 		     the process object later.  */
-		  ((struct sockaddr_in *) sa)->sin_port = sa1.sin_port;
+		  DECLARE_POINTER_ALIAS (psa, struct sockaddr_in, sa);
+		  psa->sin_port = sa1.sin_port;
 		}
 	    }
 #endif
@@ -3550,9 +3552,10 @@ connect_network_socket (Lisp_Object proc, Lisp_Object addrinfos,
 	{
 	  struct sockaddr_storage sa1;
 	  socklen_t len1 = sizeof (sa1);
-	  if (getsockname (s, (struct sockaddr *)&sa1, &len1) == 0)
+	  DECLARE_POINTER_ALIAS (psa1, struct sockaddr, &sa1);
+	  if (getsockname (s, psa1, &len1) == 0)
 	    contact = Fplist_put (contact, QClocal,
-				  conv_sockaddr_to_lisp ((struct sockaddr *)&sa1, len1));
+				  conv_sockaddr_to_lisp (psa1, len1));
 	}
 #endif
     }
@@ -4401,7 +4404,7 @@ network_interface_info (Lisp_Object ifname)
 
       for (it = ifap; it != NULL; it = it->ifa_next)
         {
-          struct sockaddr_dl *sdl = (struct sockaddr_dl *) it->ifa_addr;
+	  DECLARE_POINTER_ALIAS (sdl, struct sockaddr_dl, it->ifa_addr);
           unsigned char linkaddr[6];
           int n;
 
@@ -4722,7 +4725,7 @@ server_accept_connection (Lisp_Object server, int channel)
       {
 	args[nargs++] = procname_format_in6;
 	nargs++;
-	uint16_t *ip6 = (uint16_t *)&saddr.in6.sin6_addr;
+	DECLARE_POINTER_ALIAS (ip6, uint16_t, &saddr.in6.sin6_addr);
 	service = make_number (ntohs (saddr.in.sin_port));
 	for (int i = 0; i < 8; i++)
 	  args[nargs++] = make_number (ip6[i]);
-- 
2.9.4