From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Paul Eggert Newsgroups: gmane.emacs.devel Subject: Re: [PATCH] Fix use of sockaddr_in Date: Wed, 17 May 2017 13:38:41 -0700 Organization: UCLA Computer Science Department Message-ID: <034ef5c4-8bb1-264a-f1f4-46789d2d98c3@cs.ucla.edu> References: <83shk989r5.fsf@gnu.org> <20170513150837.31184-1-phst@google.com> <83h90o8zt9.fsf@gnu.org> <877f1kefhi.fsf@linux-m68k.org> <83a86g8siw.fsf@gnu.org> <87y3u0cyjy.fsf@linux-m68k.org> <838tm088yj.fsf@gnu.org> <8337c78qq6.fsf@gnu.org> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------6163D93BF99902D6D4559D41" X-Trace: blaine.gmane.org 1495053578 4296 195.159.176.226 (17 May 2017 20:39:38 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Wed, 17 May 2017 20:39:38 +0000 (UTC) User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.0 Cc: p.stephani2@gmail.com, emacs-devel@gnu.org To: Philipp Stephani , Eli Zaretskii , Andreas Schwab Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Wed May 17 22:39:29 2017 Return-path: Envelope-to: ged-emacs-devel@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1dB5jC-0000p6-Cb for ged-emacs-devel@m.gmane.org; Wed, 17 May 2017 22:39:26 +0200 Original-Received: from localhost ([::1]:50666 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dB5jH-0006Di-Pv for ged-emacs-devel@m.gmane.org; Wed, 17 May 2017 16:39:31 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:60575) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dB5id-0006Db-5K for emacs-devel@gnu.org; Wed, 17 May 2017 16:38:52 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dB5ib-0006Hu-H0 for emacs-devel@gnu.org; Wed, 17 May 2017 16:38:51 -0400 Original-Received: from zimbra.cs.ucla.edu ([131.179.128.68]:54402) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dB5iX-0006FB-JS; Wed, 17 May 2017 16:38:45 -0400 Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id BF0A216007A; Wed, 17 May 2017 13:38:43 -0700 (PDT) Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10032) with ESMTP id Hr00UQHUBsjc; Wed, 17 May 2017 13:38:42 -0700 (PDT) Original-Received: from localhost (localhost [127.0.0.1]) by zimbra.cs.ucla.edu (Postfix) with ESMTP id 1B583160091; Wed, 17 May 2017 13:38:42 -0700 (PDT) X-Virus-Scanned: amavisd-new at zimbra.cs.ucla.edu Original-Received: from zimbra.cs.ucla.edu ([127.0.0.1]) by localhost (zimbra.cs.ucla.edu [127.0.0.1]) (amavisd-new, port 10026) with ESMTP id aBucDnpWVrO7; Wed, 17 May 2017 13:38:41 -0700 (PDT) Original-Received: from Penguin.CS.UCLA.EDU (Penguin.CS.UCLA.EDU [131.179.64.200]) by zimbra.cs.ucla.edu (Postfix) with ESMTPSA id 7AC4316007A; Wed, 17 May 2017 13:38:41 -0700 (PDT) In-Reply-To: Content-Language: en-US X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 131.179.128.68 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:214931 Archived-At: This is a multi-part message in MIME format. --------------6163D93BF99902D6D4559D41 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit 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? --------------6163D93BF99902D6D4559D41 Content-Type: text/x-patch; name="0001-Avoid-undefined-behavior-in-struct-sockaddr.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="0001-Avoid-undefined-behavior-in-struct-sockaddr.patch" >From 2c70724a4d9809d34caefa0903012d8449b62a3c Mon Sep 17 00:00:00 2001 From: Paul Eggert 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 --------------6163D93BF99902D6D4559D41--