[PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Florian Weimer]

This avoids aliasing issues with GCC 6 in -fno-strict-aliasing
mode.  (With implicit padding, not all data is copied.)

2016-05-19  Florian Weimer  <fweimer@redhat.com>

	[BZ #20111]
	* bits/socket.h (struct sockaddr_storage): Avoid implicit padding.
	* sysdeps/mach/hurd/bits/socket.h (struct sockaddr_storage):
	Likewise.
	* sysdeps/unix/sysv/linux/bits/socket.h (struct sockaddr_storage):
	Likewise.

diff --git a/bits/socket.h b/bits/socket.h
index ab9f242..b3f2753 100644
--- a/bits/socket.h
+++ b/bits/socket.h
@@ -164,6 +164,7 @@ struct sockaddr
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
+    char __ss_padding_init[sizeof (__ss_aligntype) - __SOCKADDR_COMMON_SIZE];
     __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
   };
diff --git a/sysdeps/mach/hurd/bits/socket.h b/sysdeps/mach/hurd/bits/socket.h
index 02c5dac..8d1c2fd 100644
--- a/sysdeps/mach/hurd/bits/socket.h
+++ b/sysdeps/mach/hurd/bits/socket.h
@@ -168,6 +168,7 @@ struct sockaddr
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
+    char __ss_padding_init[sizeof (__ss_aligntype) - __SOCKADDR_COMMON_SIZE];
     __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
   };
diff --git a/sysdeps/unix/sysv/linux/bits/socket.h b/sysdeps/unix/sysv/linux/bits/socket.h
index 0581c79..448b436 100644
--- a/sysdeps/unix/sysv/linux/bits/socket.h
+++ b/sysdeps/unix/sysv/linux/bits/socket.h
@@ -166,6 +166,7 @@ struct sockaddr
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
+    char __ss_padding_init[sizeof (__ss_aligntype) - __SOCKADDR_COMMON_SIZE];
     __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
   };

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Andreas Schwab]

fweimer@redhat.com (Florian Weimer) writes:

> diff --git a/bits/socket.h b/bits/socket.h
> index ab9f242..b3f2753 100644
> --- a/bits/socket.h
> +++ b/bits/socket.h
> @@ -164,6 +164,7 @@ struct sockaddr
>  struct sockaddr_storage
>    {
>      __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
> +    char __ss_padding_init[sizeof (__ss_aligntype) - __SOCKADDR_COMMON_SIZE];

That needs to be __alignof, otherwise you change the layout, and you
need to watch out for zero padding.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Florian Weimer]

On 05/19/2016 03:40 PM, Andreas Schwab wrote:
> fweimer@redhat.com (Florian Weimer) writes:
>
>> diff --git a/bits/socket.h b/bits/socket.h
>> index ab9f242..b3f2753 100644
>> --- a/bits/socket.h
>> +++ b/bits/socket.h
>> @@ -164,6 +164,7 @@ struct sockaddr
>>  struct sockaddr_storage
>>    {
>>      __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
>> +    char __ss_padding_init[sizeof (__ss_aligntype) - __SOCKADDR_COMMON_SIZE];
>
> That needs to be __alignof, otherwise you change the layout, and you
> need to watch out for zero padding.

Hmm.  This would have to be conditional on __GNUC__.

What if we put the padding in the middle, like this?

#define _SS_PADSIZE \
   _SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype)

struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
     char __ss_padding[_SS_PADSIZE];
     __ss_aligntype __ss_align;	/* Force desired alignment.  */
   };

Then there should be no undeclared padding, and the __ss_align member 
should increase the alignment of the entire struct.

Thanks,
Florian

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> What if we put the padding in the middle, like this?
>
> #define _SS_PADSIZE \
>   _SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype)

That is different from the current definition, potentially increasing
the size of the structure.  The problem with the current definition is
that it assumes that offsetof (struct sockaddr_storage, __ss_align) ==
sizeof (__ss_aligntype), which is not guaranteed.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Florian Weimer]

On 05/19/2016 05:18 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> What if we put the padding in the middle, like this?
>>
>> #define _SS_PADSIZE \
>>   _SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype)
>
> That is different from the current definition, potentially increasing
> the size of the structure.  The problem with the current definition is
> that it assumes that offsetof (struct sockaddr_storage, __ss_align) ==
> sizeof (__ss_aligntype), which is not guaranteed.

Are you saying that before my changes, sizeof (struct sockaddr_storage) 
!= _SS_SIZE?  Which in-tree ports are affected by this?  m68k perhaps, 
because _ss_aligntype has just two-byte alignment?

(I'm not concerned about out-of-tree ports which redefine 
__SOCKADDR_COMMON, they can fix this up in any way they want.)

Thanks,
Florian

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> Are you saying that before my changes, sizeof (struct sockaddr_storage) !=
> _SS_SIZE?  Which in-tree ports are affected by this?  m68k perhaps,
> because _ss_aligntype has just two-byte alignment?

Yes, it's 126 there.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Florian Weimer]

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

On 05/19/2016 06:07 PM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> Are you saying that before my changes, sizeof (struct sockaddr_storage) !=
>> _SS_SIZE?  Which in-tree ports are affected by this?  m68k perhaps,
>> because _ss_aligntype has just two-byte alignment?
>
> Yes, it's 126 there.

Let's make this explicit.  What about the attached patch?

The test passes (on x86_64) with both GCC 5.3.1 and GCC 7 (trunk of a 
few days ago).

Thanks,
Florian

[-- Attachment #2: bug20111.patch --]
[-- Type: text/x-patch, Size: 12133 bytes --]

Make padding in struct sockaddr_storage explicit [BZ #20111]

This avoids aliasing issues with GCC 6 in -fno-strict-aliasing
mode.  (With implicit padding, not all data is copied.)

This change makes it explicit that struct sockaddr_storage is
only 126 bytes large on m68k (unlike elsewhere, where we end up
with the requested 128 bytes).  The new test case makes sure that
this does not happen on other architectures.

2016-05-20  Florian Weimer  <fweimer@redhat.com>

	[BZ #20111]
	* bits/sockaddr.h (_SS_SIZE): Define.
	* bits/socket.h (_SS_SIZE): Remove.
	(_SS_PADSIZE): Adjust to account for all padding.
	(struct sockaddr_storage): Update comment.  Avoid implicit
	padding.
	* sysdeps/mach/hurd/bits/socket.h (_SS_SIZE): Remove.
	(_SS_PADSIZE): Adjust to account for all padding.
	(struct sockaddr_storage): Update comment.  Avoid implicit
	padding.
	* sysdeps/unix/bsd/bits/sockaddr.h (_SS_SIZE): Define.
	* sysdeps/unix/sysv/linux/bits/socket.h (_SS_SIZE): Remove.
	(_SS_PADSIZE): Adjust to account for all padding.
	(struct sockaddr_storage): Update comment.  Avoid implicit
	padding.
	* sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h: New file.
	__SS_SIZE is 126 in this version.
	* inet/tst-sockaddr.c: New file.
	* inet/Makefile (tests): Add tst-sockaddr.c
	(tst-sockaddr.c): Compile with non-strict aliasing.

diff --git a/bits/sockaddr.h b/bits/sockaddr.h
index e91f837..0af58c9 100644
--- a/bits/sockaddr.h
+++ b/bits/sockaddr.h
@@ -1,4 +1,4 @@
-/* Definition of `struct sockaddr_*' common members.  Generic/4.2 BSD version.
+/* Definition of struct sockaddr_* common members and sizes, generic version.
    Copyright (C) 1995-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -36,4 +36,7 @@ typedef unsigned short int sa_family_t;
 
 #define __SOCKADDR_COMMON_SIZE	(sizeof (unsigned short int))
 
+/* Size of struct sockaddr_storage.  */
+#define _SS_SIZE 128
+
 #endif	/* bits/sockaddr.h */
diff --git a/bits/socket.h b/bits/socket.h
index ab9f242..a22fd56 100644
--- a/bits/socket.h
+++ b/bits/socket.h
@@ -152,20 +152,20 @@ struct sockaddr
 
 
 /* Structure large enough to hold any socket address (with the historical
-   exception of AF_UNIX).  We reserve 128 bytes.  */
+   exception of AF_UNIX).  */
 #if ULONG_MAX > 0xffffffff
 # define __ss_aligntype	__uint64_t
 #else
 # define __ss_aligntype	__uint32_t
 #endif
-#define _SS_SIZE	128
-#define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
+#define _SS_PADSIZE \
+  (_SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype))
 
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
-    __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
+    __ss_aligntype __ss_align;	/* Force desired alignment.  */
   };
 
 
diff --git a/inet/Makefile b/inet/Makefile
index 0e7a3c3..2207b93 100644
--- a/inet/Makefile
+++ b/inet/Makefile
@@ -50,7 +50,7 @@ aux := check_pf check_native ifreq
 
 tests := htontest test_ifindex tst-ntoa tst-ether_aton tst-network \
 	 tst-gethnm test-ifaddrs bug-if1 test-inet6_opt tst-ether_line \
-	 tst-getni1 tst-getni2 tst-inet6_rth tst-checks
+	 tst-getni1 tst-getni2 tst-inet6_rth tst-checks tst-sockaddr
 
 include ../Rules
 
@@ -84,6 +84,8 @@ CFLAGS-either_hton.c = -fexceptions
 CFLAGS-getnetgrent.c = -fexceptions
 CFLAGS-getnetgrent_r.c = -fexceptions
 
+CFLAGS-tst-sockaddr.c = -fno-strict-aliasing
+
 endif
 
 ifeq ($(build-static-nss),yes)
diff --git a/inet/tst-sockaddr.c b/inet/tst-sockaddr.c
new file mode 100644
index 0000000..4e951bd
--- /dev/null
+++ b/inet/tst-sockaddr.c
@@ -0,0 +1,118 @@
+/* Tests for socket address type definitions.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#include <netinet/in.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <stddef.h>
+
+/* This is a copy of the previous definition of struct
+   sockaddr_storage.  It is not equal to the old value of _SS_SIZE
+   (128) on all architectures.  We must stay compatible with the old
+   definition.  */
+
+#define OLD_REFERENCE_SIZE 128
+#define OLD_PADSIZE (OLD_REFERENCE_SIZE - (2 * sizeof (__ss_aligntype)))
+struct sockaddr_storage_old
+  {
+    __SOCKADDR_COMMON (old_);
+    __ss_aligntype old_align;
+    char old_padding[OLD_PADSIZE];
+  };
+
+static int
+do_test (void)
+{
+  _Static_assert (OLD_REFERENCE_SIZE >= _SS_SIZE,
+                  "old target size is not smaller than actual size");
+  _Static_assert (sizeof (struct sockaddr_storage_old)
+                  == sizeof (struct sockaddr_storage),
+                  "old and new sizes match");
+  _Static_assert (__alignof (struct sockaddr_storage_old)
+                  == __alignof (struct sockaddr_storage),
+                  "old and new alignment matches");
+  _Static_assert (offsetof (struct sockaddr_storage_old, old_family)
+                  == offsetof (struct sockaddr_storage, ss_family),
+                  "old and new family offsets match");
+  _Static_assert (sizeof (struct sockaddr_storage) == _SS_SIZE,
+                  "struct sockaddr_storage size is correct");
+
+  /* Check for POSIX compatibility requirements between struct
+     sockaddr_storage and struct sockaddr_un.  */
+  _Static_assert (sizeof (struct sockaddr_storage)
+                  >= sizeof (struct sockaddr_un),
+                  "sockaddr_storage is at least as large as sockaddr_un");
+  _Static_assert (__alignof (struct sockaddr_storage)
+                  >= __alignof (struct sockaddr_un),
+                  "sockaddr_storage is at least as aligned as sockaddr_un");
+  _Static_assert (offsetof (struct sockaddr_storage, ss_family)
+                  == offsetof (struct sockaddr_un, sun_family),
+                  "family offsets match");
+
+  /* Check that the compiler preserves bit patterns in aggregate
+     copies.  Based on <https://gcc.gnu.org/PR71120>.  */
+  _Static_assert (sizeof (struct sockaddr_storage)
+                  >= sizeof (struct sockaddr_in),
+                  "sockaddr_storage is at least as large as sockaddr_in");
+  {
+    struct sockaddr_storage addr;
+    memset (&addr, 0, sizeof (addr));
+    {
+      struct sockaddr_in *sinp = (struct sockaddr_in *)&addr;
+      sinp->sin_family = AF_INET;
+      sinp->sin_addr.s_addr = htonl (INADDR_LOOPBACK);
+      sinp->sin_port = htons (80);
+    }
+    struct sockaddr_storage copy;
+    copy = addr;
+
+    struct sockaddr_storage *p = malloc (sizeof (*p));
+    if (p == NULL)
+      {
+        printf ("error: malloc: %m\n");
+        return 1;
+      }
+    *p = copy;
+    const struct sockaddr_in *sinp = (const struct sockaddr_in *)p;
+    if (sinp->sin_family != AF_INET)
+      {
+        printf ("error: sin_family\n");
+        exit (1);
+      }
+    if (sinp->sin_addr.s_addr != htonl (INADDR_LOOPBACK))
+      {
+        printf ("error: sin_addr\n");
+        exit (1);
+      }
+    if (sinp->sin_port != htons (80))
+      {
+        printf ("error: sin_port\n");
+        exit (1);
+      }
+    free (p);
+  }
+
+  return 0;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
diff --git a/sysdeps/mach/hurd/bits/socket.h b/sysdeps/mach/hurd/bits/socket.h
index 02c5dac..257e438 100644
--- a/sysdeps/mach/hurd/bits/socket.h
+++ b/sysdeps/mach/hurd/bits/socket.h
@@ -156,20 +156,20 @@ struct sockaddr
 
 
 /* Structure large enough to hold any socket address (with the historical
-   exception of AF_UNIX).  We reserve 128 bytes.  */
+   exception of AF_UNIX).  */
 #if ULONG_MAX > 0xffffffff
 # define __ss_aligntype	__uint64_t
 #else
 # define __ss_aligntype	__uint32_t
 #endif
-#define _SS_SIZE	128
-#define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
+#define _SS_PADSIZE \
+  (_SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype))
 
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
-    __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
+    __ss_aligntype __ss_align;	/* Force desired alignment.  */
   };
 
 
diff --git a/sysdeps/unix/bsd/bits/sockaddr.h b/sysdeps/unix/bsd/bits/sockaddr.h
index aa12768..f5900f9 100644
--- a/sysdeps/unix/bsd/bits/sockaddr.h
+++ b/sysdeps/unix/bsd/bits/sockaddr.h
@@ -1,4 +1,4 @@
-/* Definition of `struct sockaddr_*' common members.  4.4 BSD version.
+/* Definition of struct sockaddr_* common members and sizes, BSD version.
    Copyright (C) 1995-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -39,4 +39,7 @@ typedef unsigned char sa_family_t;
 
 #define _HAVE_SA_LEN	1	/* We have the sa_len field.  */
 
+/* Size of struct sockaddr_storage.  */
+#define _SS_SIZE 128
+
 #endif	/* bits/sockaddr.h */
diff --git a/sysdeps/unix/sysv/linux/bits/socket.h b/sysdeps/unix/sysv/linux/bits/socket.h
index 0581c79..8fbb6be 100644
--- a/sysdeps/unix/sysv/linux/bits/socket.h
+++ b/sysdeps/unix/sysv/linux/bits/socket.h
@@ -158,16 +158,15 @@ struct sockaddr
 
 
 /* Structure large enough to hold any socket address (with the historical
-   exception of AF_UNIX).  We reserve 128 bytes.  */
+   exception of AF_UNIX).  */
 #define __ss_aligntype	unsigned long int
-#define _SS_SIZE	128
 #define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
 
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
-    __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
+    __ss_aligntype __ss_align;	/* Force desired alignment.  */
   };
 
 
diff --git a/sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h b/sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h
new file mode 100644
index 0000000..5721f99
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h
@@ -0,0 +1,42 @@
+/* Definition of struct sockaddr_* members and sizes, Linux/m68k version.
+   Copyright (C) 1995-2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/*
+ * Never include this file directly; use <sys/socket.h> instead.
+ */
+
+#ifndef _BITS_SOCKADDR_H
+#define _BITS_SOCKADDR_H	1
+
+
+/* POSIX.1g specifies this type name for the `sa_family' member.  */
+typedef unsigned short int sa_family_t;
+
+/* This macro is used to declare the initial common members
+   of the data types used for socket addresses, `struct sockaddr',
+   `struct sockaddr_in', `struct sockaddr_un', etc.  */
+
+#define	__SOCKADDR_COMMON(sa_prefix) \
+  sa_family_t sa_prefix##family
+
+#define __SOCKADDR_COMMON_SIZE	(sizeof (unsigned short int))
+
+/* Size of struct sockaddr_storage.  */
+#define _SS_SIZE 126
+
+#endif	/* bits/sockaddr.h */

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Andreas Schwab]

Florian Weimer <fweimer@redhat.com> writes:

> diff --git a/sysdeps/unix/sysv/linux/bits/socket.h b/sysdeps/unix/sysv/linux/bits/socket.h
> index 0581c79..8fbb6be 100644
> --- a/sysdeps/unix/sysv/linux/bits/socket.h
> +++ b/sysdeps/unix/sysv/linux/bits/socket.h
> @@ -158,16 +158,15 @@ struct sockaddr
>  
>  
>  /* Structure large enough to hold any socket address (with the historical
> -   exception of AF_UNIX).  We reserve 128 bytes.  */
> +   exception of AF_UNIX).  */
>  #define __ss_aligntype	unsigned long int
> -#define _SS_SIZE	128
>  #define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))

_SS_PADSIZE needs to be updated here as well.  Otherwise, looks ok.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [PATCH] Make padding in struct sockaddr_storage explicit [BZ #20111]

[Florian Weimer]

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

On 05/23/2016 10:07 AM, Andreas Schwab wrote:
> Florian Weimer <fweimer@redhat.com> writes:
>
>> diff --git a/sysdeps/unix/sysv/linux/bits/socket.h b/sysdeps/unix/sysv/linux/bits/socket.h
>> index 0581c79..8fbb6be 100644
>> --- a/sysdeps/unix/sysv/linux/bits/socket.h
>> +++ b/sysdeps/unix/sysv/linux/bits/socket.h
>> @@ -158,16 +158,15 @@ struct sockaddr
>>
>>
>>  /* Structure large enough to hold any socket address (with the historical
>> -   exception of AF_UNIX).  We reserve 128 bytes.  */
>> +   exception of AF_UNIX).  */
>>  #define __ss_aligntype	unsigned long int
>> -#define _SS_SIZE	128
>>  #define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
>
> _SS_PADSIZE needs to be updated here as well.  Otherwise, looks ok.
>
> Andreas.

Great catch, thanks.

I added some more tests to cover this (the implicit padding was outside 
the part covered by struct sockaddr_in, so the bug wasn't visible 
before).  I realized that our test harness does not deal well with 
compilation failures, so I replaced the _Static_asserts with a run-time 
check.

I'm attaching what I'm going to commit later.

Florian


[-- Attachment #2: bug20111.patch --]
[-- Type: text/x-patch, Size: 12426 bytes --]

Make padding in struct sockaddr_storage explicit [BZ #20111]

This avoids aliasing issues with GCC 6 in -fno-strict-aliasing
mode.  (With implicit padding, not all data is copied.)

This change makes it explicit that struct sockaddr_storage is
only 126 bytes large on m68k (unlike elsewhere, where we end up
with the requested 128 bytes).  The new test case makes sure that
this does not happen on other architectures.

2016-05-23  Florian Weimer  <fweimer@redhat.com>

	[BZ #20111]
	* bits/sockaddr.h (_SS_SIZE): Define.
	* bits/socket.h (_SS_SIZE): Remove.
	(_SS_PADSIZE): Adjust to account for all padding.
	(struct sockaddr_storage): Update comment.  Avoid implicit
	padding.
	* sysdeps/mach/hurd/bits/socket.h (_SS_SIZE): Remove.
	(_SS_PADSIZE): Adjust to account for all padding.
	(struct sockaddr_storage): Update comment.  Avoid implicit
	padding.
	* sysdeps/unix/bsd/bits/sockaddr.h (_SS_SIZE): Define.
	* sysdeps/unix/sysv/linux/bits/socket.h (_SS_SIZE): Remove.
	(_SS_PADSIZE): Adjust to account for all padding.
	(struct sockaddr_storage): Update comment.  Avoid implicit
	padding.
	* sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h: New file.
	__SS_SIZE is 126 in this version.
	* inet/tst-sockaddr.c: New file.
	* inet/Makefile (tests): Add tst-sockaddr.c
	(tst-sockaddr.c): Compile with non-strict aliasing.

diff --git a/bits/sockaddr.h b/bits/sockaddr.h
index e91f837..0af58c9 100644
--- a/bits/sockaddr.h
+++ b/bits/sockaddr.h
@@ -1,4 +1,4 @@
-/* Definition of `struct sockaddr_*' common members.  Generic/4.2 BSD version.
+/* Definition of struct sockaddr_* common members and sizes, generic version.
    Copyright (C) 1995-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -36,4 +36,7 @@ typedef unsigned short int sa_family_t;
 
 #define __SOCKADDR_COMMON_SIZE	(sizeof (unsigned short int))
 
+/* Size of struct sockaddr_storage.  */
+#define _SS_SIZE 128
+
 #endif	/* bits/sockaddr.h */
diff --git a/bits/socket.h b/bits/socket.h
index ab9f242..a22fd56 100644
--- a/bits/socket.h
+++ b/bits/socket.h
@@ -152,20 +152,20 @@ struct sockaddr
 
 
 /* Structure large enough to hold any socket address (with the historical
-   exception of AF_UNIX).  We reserve 128 bytes.  */
+   exception of AF_UNIX).  */
 #if ULONG_MAX > 0xffffffff
 # define __ss_aligntype	__uint64_t
 #else
 # define __ss_aligntype	__uint32_t
 #endif
-#define _SS_SIZE	128
-#define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
+#define _SS_PADSIZE \
+  (_SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype))
 
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
-    __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
+    __ss_aligntype __ss_align;	/* Force desired alignment.  */
   };
 
 
diff --git a/inet/Makefile b/inet/Makefile
index 0e7a3c3..2207b93 100644
--- a/inet/Makefile
+++ b/inet/Makefile
@@ -50,7 +50,7 @@ aux := check_pf check_native ifreq
 
 tests := htontest test_ifindex tst-ntoa tst-ether_aton tst-network \
 	 tst-gethnm test-ifaddrs bug-if1 test-inet6_opt tst-ether_line \
-	 tst-getni1 tst-getni2 tst-inet6_rth tst-checks
+	 tst-getni1 tst-getni2 tst-inet6_rth tst-checks tst-sockaddr
 
 include ../Rules
 
@@ -84,6 +84,8 @@ CFLAGS-either_hton.c = -fexceptions
 CFLAGS-getnetgrent.c = -fexceptions
 CFLAGS-getnetgrent_r.c = -fexceptions
 
+CFLAGS-tst-sockaddr.c = -fno-strict-aliasing
+
 endif
 
 ifeq ($(build-static-nss),yes)
diff --git a/inet/tst-sockaddr.c b/inet/tst-sockaddr.c
new file mode 100644
index 0000000..6383e32
--- /dev/null
+++ b/inet/tst-sockaddr.c
@@ -0,0 +1,126 @@
+/* Tests for socket address type definitions.
+   Copyright (C) 2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public License as
+   published by the Free Software Foundation; either version 2.1 of the
+   License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; see the file COPYING.LIB.  If
+   not, see <http://www.gnu.org/licenses/>.  */
+
+#include <netinet/in.h>
+#include <stdbool.h>
+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+
+/* This is a copy of the previous definition of struct
+   sockaddr_storage.  It is not equal to the old value of _SS_SIZE
+   (128) on all architectures.  We must stay compatible with the old
+   definition.  */
+
+#define OLD_REFERENCE_SIZE 128
+#define OLD_PADSIZE (OLD_REFERENCE_SIZE - (2 * sizeof (__ss_aligntype)))
+struct sockaddr_storage_old
+  {
+    __SOCKADDR_COMMON (old_);
+    __ss_aligntype old_align;
+    char old_padding[OLD_PADSIZE];
+  };
+
+static bool errors;
+
+static void
+check (bool ok, const char *message)
+{
+  if (!ok)
+    {
+      printf ("error: failed check: %s\n", message);
+      errors = true;
+    }
+}
+
+static int
+do_test (void)
+{
+  check (OLD_REFERENCE_SIZE >= _SS_SIZE,
+         "old target size is not smaller than actual size");
+  check (sizeof (struct sockaddr_storage_old)
+         == sizeof (struct sockaddr_storage),
+         "old and new sizes match");
+  check (__alignof (struct sockaddr_storage_old)
+         == __alignof (struct sockaddr_storage),
+         "old and new alignment matches");
+  check (offsetof (struct sockaddr_storage_old, old_family)
+         == offsetof (struct sockaddr_storage, ss_family),
+         "old and new family offsets match");
+  check (sizeof (struct sockaddr_storage) == _SS_SIZE,
+         "struct sockaddr_storage size");
+
+  /* Check for lack of holes in the struct definition.   */
+  check (offsetof (struct sockaddr_storage, __ss_padding)
+         == __SOCKADDR_COMMON_SIZE,
+         "implicit padding before explicit padding");
+  check (offsetof (struct sockaddr_storage, __ss_align)
+         == __SOCKADDR_COMMON_SIZE
+           + sizeof (((struct sockaddr_storage) {}).__ss_padding),
+         "implicit padding before explicit padding");
+
+  /* Check for POSIX compatibility requirements between struct
+     sockaddr_storage and struct sockaddr_un.  */
+  check (sizeof (struct sockaddr_storage) >= sizeof (struct sockaddr_un),
+         "sockaddr_storage is at least as large as sockaddr_un");
+  check (__alignof (struct sockaddr_storage)
+         >= __alignof (struct sockaddr_un),
+         "sockaddr_storage is at least as aligned as sockaddr_un");
+  check (offsetof (struct sockaddr_storage, ss_family)
+         == offsetof (struct sockaddr_un, sun_family),
+         "family offsets match");
+
+  /* Check that the compiler preserves bit patterns in aggregate
+     copies.  Based on <https://gcc.gnu.org/PR71120>.  */
+  check (sizeof (struct sockaddr_storage) >= sizeof (struct sockaddr_in),
+         "sockaddr_storage is at least as large as sockaddr_in");
+  {
+    struct sockaddr_storage addr;
+    memset (&addr, 0, sizeof (addr));
+    {
+      struct sockaddr_in *sinp = (struct sockaddr_in *)&addr;
+      sinp->sin_family = AF_INET;
+      sinp->sin_addr.s_addr = htonl (INADDR_LOOPBACK);
+      sinp->sin_port = htons (80);
+    }
+    struct sockaddr_storage copy;
+    copy = addr;
+
+    struct sockaddr_storage *p = malloc (sizeof (*p));
+    if (p == NULL)
+      {
+        printf ("error: malloc: %m\n");
+        return 1;
+      }
+    *p = copy;
+    const struct sockaddr_in *sinp = (const struct sockaddr_in *)p;
+    check (sinp->sin_family == AF_INET, "sin_family");
+    check (sinp->sin_addr.s_addr == htonl (INADDR_LOOPBACK), "sin_addr");
+    check (sinp->sin_port == htons (80), "sin_port");
+    free (p);
+  }
+
+  return errors;
+}
+
+#define TEST_FUNCTION do_test ()
+#include "../test-skeleton.c"
+
diff --git a/sysdeps/mach/hurd/bits/socket.h b/sysdeps/mach/hurd/bits/socket.h
index 02c5dac..257e438 100644
--- a/sysdeps/mach/hurd/bits/socket.h
+++ b/sysdeps/mach/hurd/bits/socket.h
@@ -156,20 +156,20 @@ struct sockaddr
 
 
 /* Structure large enough to hold any socket address (with the historical
-   exception of AF_UNIX).  We reserve 128 bytes.  */
+   exception of AF_UNIX).  */
 #if ULONG_MAX > 0xffffffff
 # define __ss_aligntype	__uint64_t
 #else
 # define __ss_aligntype	__uint32_t
 #endif
-#define _SS_SIZE	128
-#define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
+#define _SS_PADSIZE \
+  (_SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype))
 
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
-    __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
+    __ss_aligntype __ss_align;	/* Force desired alignment.  */
   };
 
 
diff --git a/sysdeps/unix/bsd/bits/sockaddr.h b/sysdeps/unix/bsd/bits/sockaddr.h
index aa12768..f5900f9 100644
--- a/sysdeps/unix/bsd/bits/sockaddr.h
+++ b/sysdeps/unix/bsd/bits/sockaddr.h
@@ -1,4 +1,4 @@
-/* Definition of `struct sockaddr_*' common members.  4.4 BSD version.
+/* Definition of struct sockaddr_* common members and sizes, BSD version.
    Copyright (C) 1995-2016 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
@@ -39,4 +39,7 @@ typedef unsigned char sa_family_t;
 
 #define _HAVE_SA_LEN	1	/* We have the sa_len field.  */
 
+/* Size of struct sockaddr_storage.  */
+#define _SS_SIZE 128
+
 #endif	/* bits/sockaddr.h */
diff --git a/sysdeps/unix/sysv/linux/bits/socket.h b/sysdeps/unix/sysv/linux/bits/socket.h
index 0581c79..50bfbc3 100644
--- a/sysdeps/unix/sysv/linux/bits/socket.h
+++ b/sysdeps/unix/sysv/linux/bits/socket.h
@@ -158,16 +158,16 @@ struct sockaddr
 
 
 /* Structure large enough to hold any socket address (with the historical
-   exception of AF_UNIX).  We reserve 128 bytes.  */
+   exception of AF_UNIX).  */
 #define __ss_aligntype	unsigned long int
-#define _SS_SIZE	128
-#define _SS_PADSIZE	(_SS_SIZE - (2 * sizeof (__ss_aligntype)))
+#define _SS_PADSIZE \
+  (_SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype))
 
 struct sockaddr_storage
   {
     __SOCKADDR_COMMON (ss_);	/* Address family, etc.  */
-    __ss_aligntype __ss_align;	/* Force desired alignment.  */
     char __ss_padding[_SS_PADSIZE];
+    __ss_aligntype __ss_align;	/* Force desired alignment.  */
   };
 
 
diff --git a/sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h b/sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h
new file mode 100644
index 0000000..5721f99
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/m68k/bits/sockaddr.h
@@ -0,0 +1,42 @@
+/* Definition of struct sockaddr_* members and sizes, Linux/m68k version.
+   Copyright (C) 1995-2016 Free Software Foundation, Inc.
+   This file is part of the GNU C Library.
+
+   The GNU C Library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   The GNU C Library 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
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with the GNU C Library; if not, see
+   <http://www.gnu.org/licenses/>.  */
+
+/*
+ * Never include this file directly; use <sys/socket.h> instead.
+ */
+
+#ifndef _BITS_SOCKADDR_H
+#define _BITS_SOCKADDR_H	1
+
+
+/* POSIX.1g specifies this type name for the `sa_family' member.  */
+typedef unsigned short int sa_family_t;
+
+/* This macro is used to declare the initial common members
+   of the data types used for socket addresses, `struct sockaddr',
+   `struct sockaddr_in', `struct sockaddr_un', etc.  */
+
+#define	__SOCKADDR_COMMON(sa_prefix) \
+  sa_family_t sa_prefix##family
+
+#define __SOCKADDR_COMMON_SIZE	(sizeof (unsigned short int))
+
+/* Size of struct sockaddr_storage.  */
+#define _SS_SIZE 126
+
+#endif	/* bits/sockaddr.h */