Unaligned memory accesses

Unaligned memory accesses

[Ludovic Courtès]

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

Hi,

I just committed the attached patch, which fixes unaligned accesses in
`socket.c' (notably, `accept' would randomly yield a "bus error" on
SPARC).  The trick is to use unions to allow the compiler to suitably
align variables.

Trying to compile with `-Wcast-align' (on SPARC) is instructive.  There
are many places, notably in `numbers.[ch]' and `vectors.[ch]', that are
potentially faulty.

Thanks,
Ludovic.


[-- Attachment #2: Socket alignment --]
[-- Type: text/x-patch, Size: 8955 bytes --]

--- orig/ChangeLog
+++ mod/ChangeLog
@@ -1,3 +1,7 @@
+2007-09-03  Ludovic Courtès  <ludo@gnu.org>
+
+	* NEWS: Mention alignment-related bug fixes.
+
 2007-09-02  Ludovic Courtès  <ludo@gnu.org>
 
 	* NEWS: Mention memory leak fix in `make-socket-address'.


--- orig/NEWS
+++ mod/NEWS
@@ -44,6 +44,7 @@
 ** Expressions like "(set! 'x #t)" no longer yield a crash
 ** Warnings about duplicate bindings now go to stderr
 ** A memory leak in `make-socket-address' was fixed
+** Alignment issues (e.g., on SPARC) in network routines were fixed
 ** Build problems on Solaris fixed
 
 * Implementation improvements


--- orig/libguile/ChangeLog
+++ mod/libguile/ChangeLog
@@ -1,3 +1,18 @@
+2007-09-03  Ludovic Courtès  <ludo@gnu.org>
+
+	Fix alignment issues which showed up at least on SPARC.
+
+	* socket.c (scm_t_max_sockaddr, scm_t_getsockopt_result): New.
+	(scm_inet_pton): Change DST to `scm_t_uint32' for correct
+	alignment.
+	(scm_getsockopt): Change OPTVAL to `scm_t_getsockopt_result' for
+	correct alignment.
+	(_scm_from_sockaddr): Change ADDRESS to `scm_t_max_sockaddr *'.
+	(scm_from_sockaddr): Cast ADDRESS to `scm_t_max_sockaddr *'.
+	(MAX_SIZE_UN, MAX_SIZE_IN6): Removed.
+	(scm_accept, scm_getsockname, scm_getpeername, scm_recvfrom):
+	Use `scm_t_max_sockaddr' instead of "char max_addr[MAX_ADDR_SIZE]".
+
 2007-09-02  Ludovic Courtès  <ludo@gnu.org>
 
 	* socket.c (scm_make_socket_address): Free C_ADDRESS after use.


--- orig/libguile/socket.c
+++ mod/libguile/socket.c
@@ -67,6 +67,26 @@
 		      + strlen ((ptr)->sun_path))
 #endif
 
+/* The largest possible socket address.  Wrapping it in a union guarantees
+   that the compiler will make it suitably aligned.  */
+typedef union
+{
+  struct sockaddr     sockaddr;
+  struct sockaddr_in  sockaddr_in;
+
+#ifdef HAVE_UNIX_DOMAIN_SOCKETS
+  struct sockaddr_un  sockaddr_un;
+#endif
+#ifdef HAVE_IPV6
+  struct sockaddr_in6 sockaddr_in6;
+#endif
+} scm_t_max_sockaddr;
+
+
+/* Maximum size of a socket address.  */
+#define MAX_ADDR_SIZE   (sizeof (scm_t_max_sockaddr))
+
+
 \f
 
 SCM_DEFINE (scm_htons, "htons", 1, 0, 0, 
@@ -344,7 +364,7 @@
 {
   int af;
   char *src;
-  char dst[16];
+  scm_t_uint32 dst[4];
   int rv, eno;
 
   af = scm_to_int (family);
@@ -359,7 +379,7 @@
   else if (rv == 0)
     SCM_MISC_ERROR ("Bad address", SCM_EOL);
   if (af == AF_INET)
-    return scm_from_ulong (ntohl (*(scm_t_uint32 *) dst));
+    return scm_from_ulong (ntohl (*dst));
   else
     return scm_from_ipv6 ((scm_t_uint8 *) dst);
 }
@@ -468,6 +488,17 @@
 #undef FUNC_NAME
 #endif
 
+/* Possible results for `getsockopt ()'.  Wrapping it into a union guarantees
+   suitable alignment.  */
+typedef union
+{
+#ifdef HAVE_STRUCT_LINGER
+  struct linger linger;
+#endif
+  size_t size;
+  int    integer;
+} scm_t_getsockopt_result;
+
 SCM_DEFINE (scm_getsockopt, "getsockopt", 3, 0, 0,
             (SCM sock, SCM level, SCM optname),
 	    "Return an option value from socket port @var{sock}.\n"
@@ -518,13 +549,8 @@
 {
   int fd;
   /* size of optval is the largest supported option.  */
-#ifdef HAVE_STRUCT_LINGER
-  char optval[sizeof (struct linger)];
-  socklen_t optlen = sizeof (struct linger);
-#else
-  char optval[sizeof (size_t)];
-  socklen_t optlen = sizeof (size_t);
-#endif
+  scm_t_getsockopt_result optval;
+  socklen_t optlen = sizeof (optval);
   int ilevel;
   int ioptname;
 
@@ -534,7 +560,7 @@
   ioptname = scm_to_int (optname);
 
   fd = SCM_FPORT_FDES (sock);
-  if (getsockopt (fd, ilevel, ioptname, (void *) optval, &optlen) == -1)
+  if (getsockopt (fd, ilevel, ioptname, (void *) &optval, &optlen) == -1)
     SCM_SYSERROR;
 
   if (ilevel == SOL_SOCKET)
@@ -543,12 +569,12 @@
       if (ioptname == SO_LINGER)
 	{
 #ifdef HAVE_STRUCT_LINGER
-	  struct linger *ling = (struct linger *) optval;
+	  struct linger *ling = (struct linger *) &optval;
 
 	  return scm_cons (scm_from_long (ling->l_onoff),
 			   scm_from_long (ling->l_linger));
 #else
-	  return scm_cons (scm_from_long (*(int *) optval),
+	  return scm_cons (scm_from_long (*(int *) &optval),
 			   scm_from_int (0));
 #endif
 	}
@@ -563,10 +589,10 @@
 #endif
 	    )
 	  {
-	    return scm_from_size_t (*(size_t *) optval);
+	    return scm_from_size_t (*(size_t *) &optval);
 	  }
     }
-  return scm_from_int (*(int *) optval);
+  return scm_from_int (*(int *) &optval);
 }
 #undef FUNC_NAME
 
@@ -1011,12 +1037,11 @@
 
 /* Put the components of a sockaddr into a new SCM vector.  */
 static SCM_C_INLINE_KEYWORD SCM
-_scm_from_sockaddr (const struct sockaddr *address, unsigned addr_size,
-		 const char *proc)
+_scm_from_sockaddr (const scm_t_max_sockaddr *address, unsigned addr_size,
+		    const char *proc)
 {
-  short int fam = address->sa_family;
-  SCM result =SCM_EOL;
-
+  SCM result = SCM_EOL;
+  short int fam = ((struct sockaddr *) address)->sa_family;
 
   switch (fam)
     {
@@ -1083,7 +1108,8 @@
 SCM
 scm_from_sockaddr (const struct sockaddr *address, unsigned addr_size)
 {
-  return (_scm_from_sockaddr (address, addr_size, "scm_from_sockaddr"));
+  return (_scm_from_sockaddr ((scm_t_max_sockaddr *) address,
+			      addr_size, "scm_from_sockaddr"));
 }
 
 /* Convert ADDRESS, an address object returned by either
@@ -1279,25 +1305,6 @@
 #undef FUNC_NAME
 
 \f
-/* calculate the size of a buffer large enough to hold any supported
-   sockaddr type.  if the buffer isn't large enough, certain system
-   calls will return a truncated address.  */
-
-#if defined (HAVE_UNIX_DOMAIN_SOCKETS)
-#define MAX_SIZE_UN sizeof (struct sockaddr_un)
-#else
-#define MAX_SIZE_UN 0
-#endif
-
-#if defined (HAVE_IPV6)
-#define MAX_SIZE_IN6 sizeof (struct sockaddr_in6)
-#else
-#define MAX_SIZE_IN6 0
-#endif
-
-#define MAX_ADDR_SIZE max (max (sizeof (struct sockaddr_in), MAX_SIZE_IN6),\
-                           MAX_SIZE_UN)
-
 SCM_DEFINE (scm_accept, "accept", 1, 0, 0, 
             (SCM sock),
 	    "Accept a connection on a bound, listening socket.\n"
@@ -1319,17 +1326,18 @@
   SCM address;
   SCM newsock;
   socklen_t addr_size = MAX_ADDR_SIZE;
-  char max_addr[MAX_ADDR_SIZE];
-  struct sockaddr *addr = (struct sockaddr *) max_addr;
+  scm_t_max_sockaddr addr;
 
   sock = SCM_COERCE_OUTPORT (sock);
   SCM_VALIDATE_OPFPORT (1, sock);
   fd = SCM_FPORT_FDES (sock);
-  newfd = accept (fd, addr, &addr_size);
+  newfd = accept (fd, (struct sockaddr *) &addr, &addr_size);
   if (newfd == -1)
     SCM_SYSERROR;
   newsock = SCM_SOCK_FD_TO_PORT (newfd);
-  address = _scm_from_sockaddr (addr, addr_size, FUNC_NAME);
+  address = _scm_from_sockaddr (&addr, addr_size,
+				FUNC_NAME);
+
   return scm_cons (newsock, address);
 }
 #undef FUNC_NAME
@@ -1343,15 +1351,15 @@
 {
   int fd;
   socklen_t addr_size = MAX_ADDR_SIZE;
-  char max_addr[MAX_ADDR_SIZE];
-  struct sockaddr *addr = (struct sockaddr *) max_addr;
+  scm_t_max_sockaddr addr;
 
   sock = SCM_COERCE_OUTPORT (sock);
   SCM_VALIDATE_OPFPORT (1, sock);
   fd = SCM_FPORT_FDES (sock);
-  if (getsockname (fd, addr, &addr_size) == -1)
+  if (getsockname (fd, (struct sockaddr *) &addr, &addr_size) == -1)
     SCM_SYSERROR;
-  return _scm_from_sockaddr (addr, addr_size, FUNC_NAME);
+
+  return _scm_from_sockaddr (&addr, addr_size, FUNC_NAME);
 }
 #undef FUNC_NAME
 
@@ -1365,15 +1373,15 @@
 {
   int fd;
   socklen_t addr_size = MAX_ADDR_SIZE;
-  char max_addr[MAX_ADDR_SIZE];
-  struct sockaddr *addr = (struct sockaddr *) max_addr;
+  scm_t_max_sockaddr addr;
 
   sock = SCM_COERCE_OUTPORT (sock);
   SCM_VALIDATE_OPFPORT (1, sock);
   fd = SCM_FPORT_FDES (sock);
-  if (getpeername (fd, addr, &addr_size) == -1)
+  if (getpeername (fd, (struct sockaddr *) &addr, &addr_size) == -1)
     SCM_SYSERROR;
-  return _scm_from_sockaddr (addr, addr_size, FUNC_NAME);
+
+  return _scm_from_sockaddr (&addr, addr_size, FUNC_NAME);
 }
 #undef FUNC_NAME
 
@@ -1509,8 +1517,7 @@
   size_t cend;
   SCM address;
   socklen_t addr_size = MAX_ADDR_SIZE;
-  char max_addr[MAX_ADDR_SIZE];
-  struct sockaddr *addr = (struct sockaddr *) max_addr;
+  scm_t_max_sockaddr addr;
 
   SCM_VALIDATE_OPFPORT (1, sock);
   fd = SCM_FPORT_FDES (sock);
@@ -1527,20 +1534,21 @@
   /* recvfrom will not necessarily return an address.  usually nothing
      is returned for stream sockets.  */
   buf = scm_i_string_writable_chars (str);
-  addr->sa_family = AF_UNSPEC;
+  ((struct sockaddr *) &addr)->sa_family = AF_UNSPEC;
   SCM_SYSCALL (rv = recvfrom (fd, buf + offset,
 			      cend - offset, flg,
-			      addr, &addr_size));
+			      (struct sockaddr *) &addr, &addr_size));
   scm_i_string_stop_writing ();
 
   if (rv == -1)
     SCM_SYSERROR;
-  if (addr->sa_family != AF_UNSPEC)
-    address = _scm_from_sockaddr (addr, addr_size, FUNC_NAME);
+  if (((struct sockaddr *) &addr)->sa_family != AF_UNSPEC)
+    address = _scm_from_sockaddr (&addr, addr_size, FUNC_NAME);
   else
     address = SCM_BOOL_F;
 
   scm_remember_upto_here_1 (str);
+
   return scm_cons (scm_from_int (rv), address);
 }
 #undef FUNC_NAME




[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Unaligned memory accesses

[Kevin Ryde]

ludo@gnu.org (Ludovic Courtès) writes:
>
> `accept' would randomly yield a "bus error" on SPARC).

Looks ok.

> +  if (((struct sockaddr *) &addr)->sa_family ...

That can use the union member instead of a cast, can it?


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Unaligned memory accesses

[Ludovic Courtès]

Hi,

Kevin Ryde <user42@zip.com.au> writes:

> ludo@gnu.org (Ludovic Courtès) writes:

>> +  if (((struct sockaddr *) &addr)->sa_family ...
>
> That can use the union member instead of a cast, can it?

No it can't.  AFAICS, there's nothing in the C standard forcing fields
of unions to be stored at offset zero when said fields have different
sizes.

Thanks,
Ludovic.


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel

Re: Unaligned memory accesses

[Kevin Ryde]

ludovic.courtes@laas.fr (Ludovic Courtès) writes:
>
> No it can't.  AFAICS, there's nothing in the C standard forcing fields
> of unions to be stored at offset zero when said fields have different
> sizes.

Union elements start at the start of the combined space.  In particular
if the elements are structs with common initial fields then any of them
can be used to access those common fields (no matter which one it was
that was stored to).


_______________________________________________
Guile-devel mailing list
Guile-devel@gnu.org
http://lists.gnu.org/mailman/listinfo/guile-devel