[pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Pedro Alves]

Building gdbserver in C++ mode shows:

  gdb/gdbserver/tracepoint.c: In function ‘void* gdb_agent_helper_thread(void*)’:
  gdb/gdbserver/tracepoint.c:7190:47: error: cannot convert ‘sockaddr_un*’ to ‘sockaddr*’ for argument ‘2’ to ‘int accept(int, sockaddr*, socklen_t*)’
	  fd = accept (listen_fd, &sockaddr, &tmp);

A few places in the tree already have an explicit cast to struct
sockaddr *, but that's a strict aliasing violation.  Instead of
propagating invalid code, fix this by using a union instead.

gdb/ChangeLog:
2015-03-07  Pedro Alves  <palves@redhat.com>

	* common/gdb_socket.h: New file.
	* ser-tcp.c: Include gdb_socket.h.  Don't include netinet/in.h nor
	sys/socket.h.
	(net_open): Use union gdb_sockaddr_u.

gdb/gdbserver/ChangeLog:
2015-03-07  Pedro Alves  <palves@redhat.com>

	* gdbreplay.c: No longer include <netinet/in.h>, <sys/socket.h>,
	or <winsock2.h> here.  Instead include "gdb_socket.h".
	(remote_open): Use union gdb_sockaddr_u.
	* remote-utils.c: No longer include <netinet/in.h>, <sys/socket.h>
	or <winsock2.h> here.  Instead include "gdb_socket.h".
	(handle_accept_event, remote_prepare): Use union gdb_sockaddr_u.
	* tracepoint.c: Include "gdb_socket.h" instead of <sys/socket.h>
	or <sys/un.h>.
	(init_named_socket, gdb_agent_helper_thread): Use union
	gdb_sockaddr_u.
---
 gdb/ChangeLog                |  7 +++++++
 gdb/gdbserver/ChangeLog      | 13 +++++++++++++
 gdb/common/gdb_socket.h      | 43 +++++++++++++++++++++++++++++++++++++++++++
 gdb/gdbserver/gdbreplay.c    | 24 ++++++++----------------
 gdb/gdbserver/remote-utils.c | 29 ++++++++++-------------------
 gdb/gdbserver/tracepoint.c   | 19 +++++++++----------
 gdb/ser-tcp.c                | 14 +++++++-------
 7 files changed, 97 insertions(+), 52 deletions(-)
 create mode 100644 gdb/common/gdb_socket.h

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index f5da3f3..522f2bb 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,5 +1,12 @@
 2015-03-07  Pedro Alves  <palves@redhat.com>
 
+	* common/gdb_socket.h: New file.
+	* ser-tcp.c: Include gdb_socket.h.  Don't include netinet/in.h nor
+	sys/socket.h.
+	(net_open): Use union gdb_sockaddr_u.
+
+2015-03-07  Pedro Alves  <palves@redhat.com>
+
 	* common/common-exceptions.c [!__cplusplus] (enum catcher_state)
 	(exceptions_state_mc_action_iter)
 	(exceptions_state_mc_action_iter_1, exceptions_state_mc_catch):
diff --git a/gdb/gdbserver/ChangeLog b/gdb/gdbserver/ChangeLog
index 70cc374..8aa38a3 100644
--- a/gdb/gdbserver/ChangeLog
+++ b/gdb/gdbserver/ChangeLog
@@ -1,5 +1,18 @@
 2015-03-07  Pedro Alves  <palves@redhat.com>
 
+	* gdbreplay.c: No longer include <netinet/in.h>, <sys/socket.h>,
+	or <winsock2.h> here.  Instead include "gdb_socket.h".
+	(remote_open): Use union gdb_sockaddr_u.
+	* remote-utils.c: No longer include <netinet/in.h>, <sys/socket.h>
+	or <winsock2.h> here.  Instead include "gdb_socket.h".
+	(handle_accept_event, remote_prepare): Use union gdb_sockaddr_u.
+	* tracepoint.c: Include "gdb_socket.h" instead of <sys/socket.h>
+	or <sys/un.h>.
+	(init_named_socket, gdb_agent_helper_thread): Use union
+	gdb_sockaddr_u.
+
+2015-03-07  Pedro Alves  <palves@redhat.com>
+
 	Adjust all callers of TRY_CATCH to use TRY/CATCH/END_CATCH
 	instead.
 
diff --git a/gdb/common/gdb_socket.h b/gdb/common/gdb_socket.h
new file mode 100644
index 0000000..a670f74
--- /dev/null
+++ b/gdb/common/gdb_socket.h
@@ -0,0 +1,43 @@
+/* Copyright (C) 2015 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#ifndef GDB_SOCKET_H
+#define GDB_SOCKET_H
+
+#if USE_WIN32API
+#include <winsock2.h>
+#else
+#include <sys/socket.h>
+#include <netinet/in.h>
+#if HAVE_SYS_UN_H
+#include <sys/un.h>
+#endif
+#endif
+
+/* Use this union instead of casts between struct sockaddr <-> struct
+   sockaddr_foo to avoid strict aliasing violations.  */
+
+union gdb_sockaddr_u
+{
+  struct sockaddr sa;
+  struct sockaddr_in sa_in;
+#if HAVE_SYS_UN_H
+  struct sockaddr_un sa_un;
+#endif
+};
+
+#endif /* GDB_SOCKET_H */
diff --git a/gdb/gdbserver/gdbreplay.c b/gdb/gdbserver/gdbreplay.c
index a02a824..bfd6f19 100644
--- a/gdb/gdbserver/gdbreplay.c
+++ b/gdb/gdbserver/gdbreplay.c
@@ -36,24 +36,16 @@
 #include <stdlib.h>
 #include <string.h>
 #include <unistd.h>
-#ifdef HAVE_NETINET_IN_H
-#include <netinet/in.h>
-#endif
-#ifdef HAVE_SYS_SOCKET_H
-#include <sys/socket.h>
-#endif
 #if HAVE_NETDB_H
 #include <netdb.h>
 #endif
 #if HAVE_NETINET_TCP_H
 #include <netinet/tcp.h>
 #endif
+#include "gdb_socket.h"
 
 #include <alloca.h>
 
-#if USE_WIN32API
-#include <winsock2.h>
-#endif
 
 #ifndef HAVE_SOCKLEN_T
 typedef int socklen_t;
@@ -188,7 +180,7 @@ remote_open (char *name)
 #endif
       char *port_str;
       int port;
-      struct sockaddr_in sockaddr;
+      union gdb_sockaddr_u sockaddr;
       socklen_t tmp;
       int tmp_desc;
 
@@ -215,16 +207,16 @@ remote_open (char *name)
       setsockopt (tmp_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
 		  sizeof (tmp));
 
-      sockaddr.sin_family = PF_INET;
-      sockaddr.sin_port = htons (port);
-      sockaddr.sin_addr.s_addr = INADDR_ANY;
+      sockaddr.sa_in.sin_family = PF_INET;
+      sockaddr.sa_in.sin_port = htons (port);
+      sockaddr.sa_in.sin_addr.s_addr = INADDR_ANY;
 
-      if (bind (tmp_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
+      if (bind (tmp_desc, &sockaddr.sa, sizeof (sockaddr.sa_in))
 	  || listen (tmp_desc, 1))
 	perror_with_name ("Can't bind address");
 
-      tmp = sizeof (sockaddr);
-      remote_desc = accept (tmp_desc, (struct sockaddr *) &sockaddr, &tmp);
+      tmp = sizeof (sockaddr.sa_in);
+      remote_desc = accept (tmp_desc, &sockaddr.sa, &tmp);
       if (remote_desc == -1)
 	perror_with_name ("Accept failed");
 
diff --git a/gdb/gdbserver/remote-utils.c b/gdb/gdbserver/remote-utils.c
index 1de86be..69f87bd 100644
--- a/gdb/gdbserver/remote-utils.c
+++ b/gdb/gdbserver/remote-utils.c
@@ -30,12 +30,6 @@
 #if HAVE_SYS_FILE_H
 #include <sys/file.h>
 #endif
-#if HAVE_NETINET_IN_H
-#include <netinet/in.h>
-#endif
-#if HAVE_SYS_SOCKET_H
-#include <sys/socket.h>
-#endif
 #if HAVE_NETDB_H
 #include <netdb.h>
 #endif
@@ -57,10 +51,7 @@
 #include <arpa/inet.h>
 #endif
 #include <sys/stat.h>
-
-#if USE_WIN32API
-#include <winsock2.h>
-#endif
+#include "gdb_socket.h"
 
 #if __QNX__
 #include <sys/iomgr.h>
@@ -153,14 +144,14 @@ enable_async_notification (int fd)
 static int
 handle_accept_event (int err, gdb_client_data client_data)
 {
-  struct sockaddr_in sockaddr;
+  union gdb_sockaddr_u sockaddr;
   socklen_t tmp;
 
   if (debug_threads)
     debug_printf ("handling possible accept event\n");
 
-  tmp = sizeof (sockaddr);
-  remote_desc = accept (listen_desc, (struct sockaddr *) &sockaddr, &tmp);
+  tmp = sizeof (sockaddr.sa_in);
+  remote_desc = accept (listen_desc, &sockaddr.sa, &tmp);
   if (remote_desc == -1)
     perror_with_name ("Accept failed");
 
@@ -195,7 +186,7 @@ handle_accept_event (int err, gdb_client_data client_data)
 
   /* Convert IP address to string.  */
   fprintf (stderr, "Remote debugging from host %s\n",
-	   inet_ntoa (sockaddr.sin_addr));
+	   inet_ntoa (sockaddr.sa_in.sin_addr));
 
   enable_async_notification (remote_desc);
 
@@ -224,7 +215,7 @@ remote_prepare (char *name)
   static int winsock_initialized;
 #endif
   int port;
-  struct sockaddr_in sockaddr;
+  union gdb_sockaddr_u sockaddr;
   socklen_t tmp;
   char *port_end;
 
@@ -269,11 +260,11 @@ remote_prepare (char *name)
   setsockopt (listen_desc, SOL_SOCKET, SO_REUSEADDR, (char *) &tmp,
 	      sizeof (tmp));
 
-  sockaddr.sin_family = PF_INET;
-  sockaddr.sin_port = htons (port);
-  sockaddr.sin_addr.s_addr = INADDR_ANY;
+  sockaddr.sa_in.sin_family = PF_INET;
+  sockaddr.sa_in.sin_port = htons (port);
+  sockaddr.sa_in.sin_addr.s_addr = INADDR_ANY;
 
-  if (bind (listen_desc, (struct sockaddr *) &sockaddr, sizeof (sockaddr))
+  if (bind (listen_desc, &sockaddr.sa, sizeof (sockaddr.sa_in))
       || listen (listen_desc, 1))
     perror_with_name ("Can't bind address");
 
diff --git a/gdb/gdbserver/tracepoint.c b/gdb/gdbserver/tracepoint.c
index 27fcf03..6c34ad4 100644
--- a/gdb/gdbserver/tracepoint.c
+++ b/gdb/gdbserver/tracepoint.c
@@ -6817,8 +6817,7 @@ run_inferior_command (char *cmd, int len)
 
 #else /* !IN_PROCESS_AGENT */
 
-#include <sys/socket.h>
-#include <sys/un.h>
+#include "gdb_socket.h"
 
 #ifndef UNIX_PATH_MAX
 #define UNIX_PATH_MAX sizeof(((struct sockaddr_un *) NULL)->sun_path)
@@ -6837,7 +6836,7 @@ static int
 init_named_socket (const char *name)
 {
   int result, fd;
-  struct sockaddr_un addr;
+  union gdb_sockaddr_u addr;
 
   result = fd = socket (PF_UNIX, SOCK_STREAM, 0);
   if (result == -1)
@@ -6846,10 +6845,10 @@ init_named_socket (const char *name)
       return -1;
     }
 
-  addr.sun_family = AF_UNIX;
+  addr.sa_un.sun_family = AF_UNIX;
 
-  strncpy (addr.sun_path, name, UNIX_PATH_MAX);
-  addr.sun_path[UNIX_PATH_MAX - 1] = '\0';
+  strncpy (addr.sa_un.sun_path, name, UNIX_PATH_MAX);
+  addr.sa_un.sun_path[UNIX_PATH_MAX - 1] = '\0';
 
   result = access (name, F_OK);
   if (result == 0)
@@ -6865,7 +6864,7 @@ init_named_socket (const char *name)
       warning ("socket %s already exists; overwriting", name);
     }
 
-  result = bind (fd, (struct sockaddr *) &addr, sizeof (addr));
+  result = bind (fd, &addr.sa, sizeof (addr.sa_un));
   if (result == -1)
     {
       warning ("bind failed: %s", strerror (errno));
@@ -7164,17 +7163,17 @@ gdb_agent_helper_thread (void *arg)
       while (1)
 	{
 	  socklen_t tmp;
-	  struct sockaddr_un sockaddr;
+	  union gdb_sockaddr_u sockaddr;
 	  int fd;
 	  char buf[1];
 	  int ret;
 	  int stop_loop = 0;
 
-	  tmp = sizeof (sockaddr);
+	  tmp = sizeof (sockaddr.sa_un);
 
 	  do
 	    {
-	      fd = accept (listen_fd, &sockaddr, &tmp);
+	      fd = accept (listen_fd, &sockaddr.sa, &tmp);
 	    }
 	  /* It seems an ERESTARTSYS can escape out of accept.  */
 	  while (fd == -512 || (fd == -1 && errno == EINTR));
diff --git a/gdb/ser-tcp.c b/gdb/ser-tcp.c
index 9c3dcf4..648f1fb 100644
--- a/gdb/ser-tcp.c
+++ b/gdb/ser-tcp.c
@@ -37,6 +37,8 @@
 
 #include <sys/time.h>
 
+#include "gdb_socket.h"
+
 #ifdef USE_WIN32API
 #include <winsock2.h>
 #ifndef ETIMEDOUT
@@ -45,10 +47,8 @@
 #define close(fd) closesocket (fd)
 #define ioctl ioctlsocket
 #else
-#include <netinet/in.h>
 #include <arpa/inet.h>
 #include <netdb.h>
-#include <sys/socket.h>
 #include <netinet/tcp.h>
 #endif
 
@@ -159,7 +159,7 @@ net_open (struct serial *scb, const char *name)
   int n, port, tmp;
   int use_udp;
   struct hostent *hostent;
-  struct sockaddr_in sockaddr;
+  union gdb_sockaddr_u sockaddr;
 #ifdef USE_WIN32API
   u_long ioarg;
 #else
@@ -199,9 +199,9 @@ net_open (struct serial *scb, const char *name)
       return -1;
     }
 
-  sockaddr.sin_family = PF_INET;
-  sockaddr.sin_port = htons (port);
-  memcpy (&sockaddr.sin_addr.s_addr, hostent->h_addr,
+  sockaddr.sa_in.sin_family = PF_INET;
+  sockaddr.sa_in.sin_port = htons (port);
+  memcpy (&sockaddr.sa_in.sin_addr.s_addr, hostent->h_addr,
 	  sizeof (struct in_addr));
 
  retry:
@@ -220,7 +220,7 @@ net_open (struct serial *scb, const char *name)
 
   /* Use Non-blocking connect.  connect() will return 0 if connected
      already.  */
-  n = connect (scb->fd, (struct sockaddr *) &sockaddr, sizeof (sockaddr));
+  n = connect (scb->fd, &sockaddr.sa, sizeof (sockaddr.sa_in));
 
   if (n < 0)
     {
-- 
1.9.3

Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Eli Zaretskii]

> From: Pedro Alves <palves@redhat.com>
> Date: Sat,  7 Mar 2015 17:44:26 +0000
> 
> Building gdbserver in C++ mode shows:
> 
>   gdb/gdbserver/tracepoint.c: In function ‘void* gdb_agent_helper_thread(void*)’:
>   gdb/gdbserver/tracepoint.c:7190:47: error: cannot convert ‘sockaddr_un*’ to ‘sockaddr*’ for argument ‘2’ to ‘int accept(int, sockaddr*, socklen_t*)’
> 	  fd = accept (listen_fd, &sockaddr, &tmp);
> 
> A few places in the tree already have an explicit cast to struct
> sockaddr *, but that's a strict aliasing violation.  Instead of
> propagating invalid code, fix this by using a union instead.

Yuck!  Isn't there a better way?  Why do we have the original problem
to begin with, i.e. where did the incompatible data type come from?

(Does it even make sense to work around C++ restrictions while
converting code to C++?)

Thanks.

Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Pedro Alves]

On 03/07/2015 06:00 PM, Eli Zaretskii wrote:
>> From: Pedro Alves <palves@redhat.com>
>> Date: Sat,  7 Mar 2015 17:44:26 +0000
>>
>> Building gdbserver in C++ mode shows:
>>
>>   gdb/gdbserver/tracepoint.c: In function ‘void* gdb_agent_helper_thread(void*)’:
>>   gdb/gdbserver/tracepoint.c:7190:47: error: cannot convert ‘sockaddr_un*’ to ‘sockaddr*’ for argument ‘2’ to ‘int accept(int, sockaddr*, socklen_t*)’
>> 	  fd = accept (listen_fd, &sockaddr, &tmp);
>>
>> A few places in the tree already have an explicit cast to struct
>> sockaddr *, but that's a strict aliasing violation.  Instead of
>> propagating invalid code, fix this by using a union instead.
> 
> Yuck!  Isn't there a better way?  Why do we have the original problem
> to begin with, i.e. where did the incompatible data type come from?

Those are BSD socket types, they've been this way ever since BSD
invented them.  The structs are not type compatible, even though
they have some common fields that are are put at the same offsets,
by design.

See e.g.:

  http://stackoverflow.com/questions/1429645/how-to-cast-sockaddr-storage-and-avoid-breaking-strict-aliasing-rules

Lots of packages fixed this around the gcc 4.4 era, but gdb managed
to never triggers the warnings.  See e.g., (from a quick google search):

  http://comments.gmane.org/gmane.network.inn/8891
  http://pidgin.im/pipermail/commits/2010-April/016956.html

> (Does it even make sense to work around C++ restrictions while
> converting code to C++?)

It's not a C++ restriction.  The old code was invalid C code.

Thanks,
Pedro Alves

Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Eli Zaretskii]

> Date: Sat, 07 Mar 2015 18:20:18 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> Those are BSD socket types, they've been this way ever since BSD
> invented them.  The structs are not type compatible, even though
> they have some common fields that are are put at the same offsets,
> by design.
> 
> See e.g.:
> 
>   http://stackoverflow.com/questions/1429645/how-to-cast-sockaddr-storage-and-avoid-breaking-strict-aliasing-rules

IMO, the right way to handle this is to have our own struct (NOT
union!) with the data, and fill the correct struct from it, field by
field, when we need to pass it to a library function.

A union is just a type-cast in disguise; if using it is wrong, we
shouldn't try.  (And what if one of these days GCC will acquire a
capability to see through them?)

> It's not a C++ restriction.  The old code was invalid C code.

I'm talking about the new code.

Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Pedro Alves]

On 03/07/2015 06:52 PM, Eli Zaretskii wrote:
>> Date: Sat, 07 Mar 2015 18:20:18 +0000
>> From: Pedro Alves <palves@redhat.com>
>> CC: gdb-patches@sourceware.org
>>
>> Those are BSD socket types, they've been this way ever since BSD
>> invented them.  The structs are not type compatible, even though
>> they have some common fields that are are put at the same offsets,
>> by design.
>>
>> See e.g.:
>>
>>   http://stackoverflow.com/questions/1429645/how-to-cast-sockaddr-storage-and-avoid-breaking-strict-aliasing-rules
> 
> IMO, the right way to handle this is to have our own struct (NOT
> union!) with the data, and fill the correct struct from it, field by
> field, when we need to pass it to a library function.

But that doesn't really make sense in this context.
These struct sockaddr_foo types aren't ours, they're pretty much
hard core to the sockets library.  You _need_ to fill in a
struct sockaddr_foo, and pass that to bind/accept, but disguised
as a "struct sockaddr".  That's just how those calls work.  It's
impossible to fill in a struct sockaddr with the data, because
struct sockaddr does not have the necessary fields.  E.g.:

(top-gdb) p sizeof (struct sockaddr)
$3 = 16
(top-gdb) p sizeof (struct sockaddr_in)
$2 = 16
(top-gdb) p sizeof (struct sockaddr_in6)
$1 = 28
(top-gdb) p sizeof (struct sockaddr_un)
$4 = 110

if 'struct sockaddr' HAD been defined like:

struct sockaddr {
    sa_family_t sa_family;
}

and then struct sockaddr_foos had been defined as "extending"
struct sockaddr, like e.g.:

struct sockaddr_in6 {
    /* Extend a sockaddr */
    struct sockaddr sa;

    /* ... and add more data */
    in_port_t sin6_port;
    uint32_t sin6_flowinfo;
    struct in6_addr sin6_addr;
    uint32_t sin6_scope_id;
}

That is, the first field of the structure had been the
abstract "struct sockaddr", then it'd be OK to cast struct sockaddr_foo
to "struct sockaddr".  C allows that.

(Another possibility would have been for bind/accept to
take a "sa_family_t *sa_family" as argument instead of
"struct sockaddr *", same thing.)

But, they're not.  That ship has sailed many years ago.
What we have is types like these (from gdb's 'ptype',
typedefs may be missing):

struct sockaddr {
    sa_family_t sa_family;
    char sa_data[14];
}

struct sockaddr_in {
    sa_family_t sin_family;
    in_port_t sin_port;
    struct in_addr sin_addr;
    unsigned char sin_zero[8];
}

struct sockaddr_in6 {
    sa_family_t sin6_family;
    in_port_t sin6_port;
    uint32_t sin6_flowinfo;
    struct in6_addr sin6_addr;
    uint32_t sin6_scope_id;
}

struct sockaddr_un {
    sa_family_t sun_family;
    char sun_path[108];
}

And thus casting between "struct sockaddr *"
and "struct sockaddr_in6 *" is invalid.  These types can't
alias, thus with this:

 struct sockaddr_in *addr_in = ...;
 struct sockaddr *addr = (struct sockaddr *) addr_in;

 addr->sa_family = 1;
 addr_in->sin_family = 2;
 if (addr->sa_family != 1) {
    abort ();
 }

the compiler is free to completely eliminate the 'if'.

> A union is just a type-cast in disguise; if using it is wrong, we
> shouldn't try.  (And what if one of these days GCC will acquire a
> capability to see through them?)
> 

As discussed in that stackexchange page, it won't.  It's
implementation-defined till C90 whether accessing a different union member
from the one last used to store a value was valid, a.k.a., type-punning.

In GCC's implementation (and others' I believe), it's valid.
Please see:

 https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation
and:
 https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Optimize-Options.html#Type-punning

And then the C99 standard changed its wording to make it valid everywhere.

See DR283 (defect report):
 http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm

And C99 TC3, section 6.5, paragraph 7:
 www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf

Casts are invalid.  Unions work in practice with old compilers,
and are required to work in "modern" (old by now) compilers.  So
really unions are the proper solution for this, however unfortunate.

>> It's not a C++ restriction.  The old code was invalid C code.
> 
> I'm talking about the new code.

Then I don't understand what you're saying, sorry.  This has nothing
to do with C++.  C++ happened to catch it due to the missing cast.  I
could have added the cast to that one place that missed it, but that
would just be propagating both invalid C++ _and_ invalid _C_ code.

Thanks,
Pedro Alves

Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Pedro Alves]

On 03/09/2015 10:45 AM, Pedro Alves wrote:
> On 03/07/2015 06:52 PM, Eli Zaretskii wrote:
>>> Date: Sat, 07 Mar 2015 18:20:18 +0000
>>> From: Pedro Alves <palves@redhat.com>
>>> CC: gdb-patches@sourceware.org
>>>
>>> Those are BSD socket types, they've been this way ever since BSD
>>> invented them.  The structs are not type compatible, even though
>>> they have some common fields that are are put at the same offsets,
>>> by design.
>>>
>>> See e.g.:
>>>
>>>   http://stackoverflow.com/questions/1429645/how-to-cast-sockaddr-storage-and-avoid-breaking-strict-aliasing-rules
>>
>> IMO, the right way to handle this is to have our own struct (NOT
>> union!) with the data, and fill the correct struct from it, field by
>> field, when we need to pass it to a library function.
> 
> But that doesn't really make sense in this context.
> These struct sockaddr_foo types aren't ours, they're pretty much
> hard core to the sockets library.  You _need_ to fill in a
> struct sockaddr_foo, and pass that to bind/accept, but disguised
> as a "struct sockaddr".  That's just how those calls work.  It's
> impossible to fill in a struct sockaddr with the data, because
> struct sockaddr does not have the necessary fields.  E.g.:
> 
> (top-gdb) p sizeof (struct sockaddr)
> $3 = 16
> (top-gdb) p sizeof (struct sockaddr_in)
> $2 = 16
> (top-gdb) p sizeof (struct sockaddr_in6)
> $1 = 28
> (top-gdb) p sizeof (struct sockaddr_un)
> $4 = 110
> 
> if 'struct sockaddr' HAD been defined like:
> 
> struct sockaddr {
>     sa_family_t sa_family;
> }
> 
> and then struct sockaddr_foos had been defined as "extending"
> struct sockaddr, like e.g.:
> 
> struct sockaddr_in6 {
>     /* Extend a sockaddr */
>     struct sockaddr sa;
> 
>     /* ... and add more data */
>     in_port_t sin6_port;
>     uint32_t sin6_flowinfo;
>     struct in6_addr sin6_addr;
>     uint32_t sin6_scope_id;
> }
> 
> That is, the first field of the structure had been the
> abstract "struct sockaddr", then it'd be OK to cast struct sockaddr_foo
> to "struct sockaddr".  C allows that.
> 
> (Another possibility would have been for bind/accept to
> take a "sa_family_t *sa_family" as argument instead of
> "struct sockaddr *", same thing.)
> 
> But, they're not.  That ship has sailed many years ago.
> What we have is types like these (from gdb's 'ptype',
> typedefs may be missing):
> 
> struct sockaddr {
>     sa_family_t sa_family;
>     char sa_data[14];
> }
> 
> struct sockaddr_in {
>     sa_family_t sin_family;
>     in_port_t sin_port;
>     struct in_addr sin_addr;
>     unsigned char sin_zero[8];
> }
> 
> struct sockaddr_in6 {
>     sa_family_t sin6_family;
>     in_port_t sin6_port;
>     uint32_t sin6_flowinfo;
>     struct in6_addr sin6_addr;
>     uint32_t sin6_scope_id;
> }
> 
> struct sockaddr_un {
>     sa_family_t sun_family;
>     char sun_path[108];
> }
> 
> And thus casting between "struct sockaddr *"
> and "struct sockaddr_in6 *" is invalid.  These types can't
> alias, thus with this:
> 
>  struct sockaddr_in *addr_in = ...;
>  struct sockaddr *addr = (struct sockaddr *) addr_in;
> 
>  addr->sa_family = 1;
>  addr_in->sin_family = 2;
>  if (addr->sa_family != 1) {
>     abort ();
>  }
> 
> the compiler is free to completely eliminate the 'if'.
> 
>> A union is just a type-cast in disguise; if using it is wrong, we
>> shouldn't try.  (And what if one of these days GCC will acquire a
>> capability to see through them?)
>>
> 
> As discussed in that stackexchange page, it won't.  It's
> implementation-defined till C90 whether accessing a different union member
> from the one last used to store a value was valid, a.k.a., type-punning.
> 
> In GCC's implementation (and others' I believe), it's valid.
> Please see:
> 
>  https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Structures-unions-enumerations-and-bit-fields-implementation.html#Structures-unions-enumerations-and-bit-fields-implementation
> and:
>  https://gcc.gnu.org/onlinedocs/gcc-4.9.0/gcc/Optimize-Options.html#Type-punning
> 
> And then the C99 standard changed its wording to make it valid everywhere.
> 
> See DR283 (defect report):
>  http://www.open-std.org/jtc1/sc22/wg14/www/docs/dr_283.htm
> 
> And C99 TC3, section 6.5, paragraph 7:
>  www.open-std.org/jtc1/sc22/wg14/www/docs/n1256.pdf
> 
> Casts are invalid.  Unions work in practice with old compilers,
> and are required to work in "modern" (old by now) compilers.  So
> really unions are the proper solution for this, however unfortunate.
> 
>>> It's not a C++ restriction.  The old code was invalid C code.
>>
>> I'm talking about the new code.
> 
> Then I don't understand what you're saying, sorry.  This has nothing
> to do with C++.  C++ happened to catch it due to the missing cast.  I
> could have added the cast to that one place that missed it, but that
> would just be propagating both invalid C++ _and_ invalid _C_ code.

With all that said, I'm having second thoughts on this ...  Per
6.3.2.3 ("A pointer to an object or incomplete type may be converted to
a pointer to a different object or incomplete type"), as long as we don't
actually access the fields of sockaddr through the "struct sockaddr *" pointer,
then the cast should be OK.  In the bind/accept cast, it should be the
internals of those functions that need the union trick.  Gosh, what a mess.

So I'll revert the union patch, and apply the original one that added
the missing cast to tracepoint.c...

Thanks,
Pedro Alves

Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Pedro Alves]

On 03/09/2015 11:10 AM, Pedro Alves wrote:

> With all that said, I'm having second thoughts on this ...  Per
> 6.3.2.3 ("A pointer to an object or incomplete type may be converted to
> a pointer to a different object or incomplete type"), as long as we don't
> actually access the fields of sockaddr through the "struct sockaddr *" pointer,
> then the cast should be OK.  In the bind/accept cast, it should be the
> internals of those functions that need the union trick.  Gosh, what a mess.
> 
> So I'll revert the union patch, and apply the original one that added
> the missing cast to tracepoint.c...

Done.  Sorry for the noise...

Thanks,
Pedro Alves

Re: [pushed] Fix struct sockaddr/sockaddr_in/sockaddr_un strict aliasing violations

[Eli Zaretskii]

> Date: Mon, 09 Mar 2015 11:38:53 +0000
> From: Pedro Alves <palves@redhat.com>
> CC: gdb-patches@sourceware.org
> 
> On 03/09/2015 11:10 AM, Pedro Alves wrote:
> 
> > With all that said, I'm having second thoughts on this ...  Per
> > 6.3.2.3 ("A pointer to an object or incomplete type may be converted to
> > a pointer to a different object or incomplete type"), as long as we don't
> > actually access the fields of sockaddr through the "struct sockaddr *" pointer,
> > then the cast should be OK.  In the bind/accept cast, it should be the
> > internals of those functions that need the union trick.  Gosh, what a mess.
> > 
> > So I'll revert the union patch, and apply the original one that added
> > the missing cast to tracepoint.c...
> 
> Done.  Sorry for the noise...

I should be sorry first ;-)