Re: sockaddr_storage and GCC 6.1

Re: sockaddr_storage and GCC 6.1

[William Ahern]

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

I forgot to mention that I'm not subscribed to the list.

I was able to build GCC 6.1 last night, but on my OS X desktop. My Alpine
Linux development environment doesn't have the disk space. My test code
manually reproduced the structure definitions.

Both of the patches proposed appear to work, at least with strict aliasing
disabled. Attached is the source code I used to verify. Here's the output of
`make test`.

== ss-darwin-no-strict-aliasing ==
.ss_len:     0,1
.ss_family:  1,2
.__ss_pad1:  2,8
.__ss_align: 8,16
.__ss_pad2:  16,128
.sin_family: 1,2
.sin_port:   2,4
.sin_addr:   4,8
0x7f000001 (OKAY)

== ss-darwin-strict-aliasing ==
.ss_len:     0,1
.ss_family:  1,2
.__ss_pad1:  2,8
.__ss_align: 8,16
.__ss_pad2:  16,128
.sin_family: 1,2
.sin_port:   2,4
.sin_addr:   4,8
0x00000000 (FAIL)

== ss-musl0-no-strict-aliasing ==
.ss_family:    0,2
.__ss_align:   8,16
.__ss_padding: 16,128
.sin_family: 0,2
.sin_port:   2,4
.sin_addr:   4,8
0x00000000 (FAIL)

== ss-musl0-strict-aliasing ==
.ss_family:    0,2
.__ss_align:   8,16
.__ss_padding: 16,128
.sin_family: 0,2
.sin_port:   2,4
.sin_addr:   4,8
0x00000000 (FAIL)

== ss-musl1-no-strict-aliasing ==
.ss_family:       0,2
.__ss_family_pad: 2,8
.__ss_align:      8,16
.__ss_padding:    16,128
.sin_family: 0,2
.sin_port:   2,4
.sin_addr:   4,8
0x7f000001 (OKAY)

== ss-musl1-strict-aliasing ==
.ss_family:       0,2
.__ss_family_pad: 2,8
.__ss_align:      8,16
.__ss_padding:    16,128
.sin_family: 0,2
.sin_port:   2,4
.sin_addr:   4,8
0x00000000 (FAIL)

== ss-musl2-no-strict-aliasing ==
.ss_family:    0,2
.__ss_padding: 2,120
.__ss_align:   120,128
.sin_family: 0,2
.sin_port:   2,4
.sin_addr:   4,8
0x7f000001 (OKAY)

== ss-musl2-strict-aliasing ==
.ss_family:    0,2
.__ss_padding: 2,120
.__ss_align:   120,128
.sin_family: 0,2
.sin_port:   2,4
.sin_addr:   4,8
0x00000000 (FAIL)


[-- Attachment #2: Makefile --]
[-- Type: text/plain, Size: 995 bytes --]

CC=/usr/local/gcc6/bin/gcc
CPPFLAGS=
CFLAGS=-O2 -Wall -Wextra -std=c11

all: ss-darwin-strict-aliasing ss-darwin-no-strict-aliasing
all: ss-musl0-strict-aliasing ss-musl0-no-strict-aliasing
all: ss-musl1-strict-aliasing ss-musl1-no-strict-aliasing
all: ss-musl2-strict-aliasing ss-musl2-no-strict-aliasing

ss-darwin-strict-aliasing ss-darwin-no-strict-aliasing: ss.c
	$(CC) -o $@ ss.c $(CPPFLAGS) $(CFLAGS) -f$(@:ss-darwin-%=%)

ss-musl0-strict-aliasing ss-musl0-no-strict-aliasing: ss.c
	$(CC) -o $@ ss.c $(CPPFLAGS) -DMUSL_PATCH=0 $(CFLAGS) -f$(@:ss-musl0-%=%)

ss-musl1-strict-aliasing ss-musl1-no-strict-aliasing: ss.c
	$(CC) -o $@ ss.c $(CPPFLAGS) -DMUSL_PATCH=1 $(CFLAGS) -f$(@:ss-musl1-%=%)

ss-musl2-strict-aliasing ss-musl2-no-strict-aliasing: ss.c
	$(CC) -o $@ ss.c $(CPPFLAGS) -DMUSL_PATCH=2 $(CFLAGS) -f$(@:ss-musl2-%=%)

test: all
	@for T in ss-darwin* ss-musl*; do \
		printf "== %s ==\n" "$${T}"; \
		"./$${T}"; \
		printf "\n"; \
	done

clean:
	rm -f -- ./ss-darwin* ./ss-musl*

[-- Attachment #3: ss.c --]
[-- Type: text/plain, Size: 3376 bytes --]

#include <stddef.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include <err.h>

#define vectorof(T, m) offsetof(T, m), (offsetof(T, m) + sizeof (*(T *)0).m)
#define ss_vectorof(m) vectorof(struct sockaddr_storage, m)
#define sin_vectorof(m) vectorof(struct sockaddr_in, m)

#if !defined MUSL_PATCH
#include <arpa/inet.h>
#include <netinet/in.h>
#include <sys/socket.h>

#if !__APPLE__
#error exected Darwin
#endif

static void
ss_layout(void)
{
	printf(".ss_len:     %zu,%zu\n", ss_vectorof(ss_len));
	printf(".ss_family:  %zu,%zu\n", ss_vectorof(ss_family));
	printf(".__ss_pad1:  %zu,%zu\n", ss_vectorof(__ss_pad1));
	printf(".__ss_align: %zu,%zu\n", ss_vectorof(__ss_align));
	printf(".__ss_pad2:  %zu,%zu\n", ss_vectorof(__ss_pad2));
}
#else

typedef unsigned socklen_t;
typedef unsigned short sa_family_t;

#if MUSL_PATCH == 2
struct sockaddr_storage {
	sa_family_t ss_family;
	char __ss_padding[128-sizeof(long)-sizeof(sa_family_t)];
	unsigned long __ss_align;
};

static void
ss_layout(void)
{
	printf(".ss_family:    %zu,%zu\n", ss_vectorof(ss_family));
	printf(".__ss_padding: %zu,%zu\n", ss_vectorof(__ss_padding));
	printf(".__ss_align:   %zu,%zu\n", ss_vectorof(__ss_align));
}
#elif MUSL_PATCH == 1
struct sockaddr_storage {
	sa_family_t ss_family;
	char __ss_family_pad[sizeof(long)-sizeof(sa_family_t)];
	unsigned long __ss_align;
	char __ss_padding[128-2*sizeof(unsigned long)];
};

static void
ss_layout(void)
{
	printf(".ss_family:       %zu,%zu\n", ss_vectorof(ss_family));
	printf(".__ss_family_pad: %zu,%zu\n", ss_vectorof(__ss_family_pad));
	printf(".__ss_align:      %zu,%zu\n", ss_vectorof(__ss_align));
	printf(".__ss_padding:    %zu,%zu\n", ss_vectorof(__ss_padding));
}
#else
struct sockaddr_storage {
	sa_family_t ss_family;
	unsigned long __ss_align;
	char __ss_padding[128-2*sizeof(unsigned long)];
};

static void
ss_layout(void)
{
	printf(".ss_family:    %zu,%zu\n", ss_vectorof(ss_family));
	printf(".__ss_align:   %zu,%zu\n", ss_vectorof(__ss_align));
	printf(".__ss_padding: %zu,%zu\n", ss_vectorof(__ss_padding));
}
#endif
                                        
typedef uint16_t in_port_t;
typedef uint32_t in_addr_t;
struct in_addr { in_addr_t s_addr; };
struct sockaddr_in {
	sa_family_t sin_family;
	in_port_t sin_port;
	struct in_addr sin_addr;
	uint8_t sin_zero[8];
};
#define INADDR_LOOPBACK UINT32_C(0x7f000001)
#define AF_INET 2
#undef htonl
#undef htons
#undef ntohl
#undef ntohs

uint32_t htonl(uint32_t);
uint16_t htons(uint16_t);
uint32_t ntohl(uint32_t);
uint16_t ntohs(uint16_t);
#endif

static void
sin_layout(void)
{
	printf(".sin_family: %zu,%zu\n", sin_vectorof(sin_family));
	printf(".sin_port:   %zu,%zu\n", sin_vectorof(sin_port));
	printf(".sin_addr:   %zu,%zu\n", sin_vectorof(sin_addr));
}

struct dbg_listener {
	struct sockaddr_storage addr;
};

int
main(void)
{
	struct dbg_listener *l;
	struct sockaddr_storage ss, *ss2, ss3;

	ss_layout();
	sin_layout();

	memset(&ss3, 0, sizeof ss3);
	ss3.ss_family = AF_INET;
	((struct sockaddr_in *)&ss3)->sin_addr.s_addr = htonl(INADDR_LOOPBACK);

	ss2 = &ss3;
	ss = *ss2;

	l = calloc(1, sizeof *l);
	l->addr = ss;

	printf("0x%.8x (%s)\n",
	    ntohl(((struct sockaddr_in *)(&l->addr))->sin_addr.s_addr),
	    (ntohl(((struct sockaddr_in *)(&l->addr))->sin_addr.s_addr) == INADDR_LOOPBACK)?
	        "OKAY" : "FAIL");

	return 0;
}

Re: sockaddr_storage and GCC 6.1

[Rich Felker]

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

On Tue, May 24, 2016 at 06:55:48PM -0400, Rich Felker wrote:
> On Tue, May 24, 2016 at 06:36:02PM -0400, Rich Felker wrote:
> > On Tue, May 24, 2016 at 03:07:35PM -0700, William Ahern wrote:
> > > GCC 6.1 more aggressively decomposes aggregate assignments into a series of
> > > scalar member assignments. This has uncovered an issue with glibc's layout
> > > of struct sockaddr_storage, which has a padding hole from offsets 2 to 8,
> > > precisely where .sin_port and .sin_addr are in struct sockaddr_in.
> > > 
> > >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71120
> > > 
> > > musl shares this same issue. Specifically, the __ss_align member with an
> > > 8-byte alignment on LP64 archs. You can track the glibc resolution at
> > > 
> > >   https://sourceware.org/bugzilla/show_bug.cgi?id=20111
> > > 
> > > Or not track it. Reasonable folks can disagree regarding many aspects of
> > > this issue, but I thought it worthwhile to bring to people's attention.
> > 
> > I maintain that it's a bug (violation of effective type rules) for a
> > program to attempt to copy sockaddr types using sockaddr_storage, but
> > this is a nasty application bug to track down (usually silent
> > breakage) that's worth avoiding since it's easy. Does the attached
> > patch work?
> > 
> > I don't think we should even consider the sorts of may_alias hacks
> > glibc/gcc folks are discussing, though. There's already a gcc option
> > for compiling broken code like that; it's called -fno-strict-aliasing.
> > 
> > Rich
> 
> > diff --git a/include/sys/socket.h b/include/sys/socket.h
> > index 6788375..d2bd5df 100644
> > --- a/include/sys/socket.h
> > +++ b/include/sys/socket.h
> > @@ -286,7 +286,7 @@ struct sockaddr
> >  
> >  struct sockaddr_storage
> >  {
> > -	sa_family_t ss_family;
> > +	sa_family_t ss_family, __ss_family_pad;
> >  	unsigned long __ss_align;
> >  	char __ss_padding[128-2*sizeof(unsigned long)];
> >  };
> 
> This is wrong for 64-bit archs; new version attached.
> 
> Rich

> diff --git a/include/sys/socket.h b/include/sys/socket.h
> index 6788375..c7f244a 100644
> --- a/include/sys/socket.h
> +++ b/include/sys/socket.h
> @@ -287,6 +287,7 @@ struct sockaddr
>  struct sockaddr_storage
>  {
>  	sa_family_t ss_family;
> +	char __ss_family_pad[sizeof(long)-sizeof(sa_family_t)];
>  	unsigned long __ss_align;
>  	char __ss_padding[128-2*sizeof(unsigned long)];
>  };

And here's a potentially simpler version.

Rich

[-- Attachment #2: sockaddr_storage_v3.diff --]
[-- Type: text/plain, Size: 402 bytes --]

diff --git a/include/sys/socket.h b/include/sys/socket.h
index 6788375..4247915 100644
--- a/include/sys/socket.h
+++ b/include/sys/socket.h
@@ -287,8 +287,8 @@ struct sockaddr
 struct sockaddr_storage
 {
 	sa_family_t ss_family;
+	char __ss_padding[128-sizeof(long)-sizeof(sa_family_t)];
 	unsigned long __ss_align;
-	char __ss_padding[128-2*sizeof(unsigned long)];
 };
 
 int socket (int, int, int);

Re: sockaddr_storage and GCC 6.1

[Rich Felker]

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

On Tue, May 24, 2016 at 06:36:02PM -0400, Rich Felker wrote:
> On Tue, May 24, 2016 at 03:07:35PM -0700, William Ahern wrote:
> > GCC 6.1 more aggressively decomposes aggregate assignments into a series of
> > scalar member assignments. This has uncovered an issue with glibc's layout
> > of struct sockaddr_storage, which has a padding hole from offsets 2 to 8,
> > precisely where .sin_port and .sin_addr are in struct sockaddr_in.
> > 
> >   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71120
> > 
> > musl shares this same issue. Specifically, the __ss_align member with an
> > 8-byte alignment on LP64 archs. You can track the glibc resolution at
> > 
> >   https://sourceware.org/bugzilla/show_bug.cgi?id=20111
> > 
> > Or not track it. Reasonable folks can disagree regarding many aspects of
> > this issue, but I thought it worthwhile to bring to people's attention.
> 
> I maintain that it's a bug (violation of effective type rules) for a
> program to attempt to copy sockaddr types using sockaddr_storage, but
> this is a nasty application bug to track down (usually silent
> breakage) that's worth avoiding since it's easy. Does the attached
> patch work?
> 
> I don't think we should even consider the sorts of may_alias hacks
> glibc/gcc folks are discussing, though. There's already a gcc option
> for compiling broken code like that; it's called -fno-strict-aliasing.
> 
> Rich

> diff --git a/include/sys/socket.h b/include/sys/socket.h
> index 6788375..d2bd5df 100644
> --- a/include/sys/socket.h
> +++ b/include/sys/socket.h
> @@ -286,7 +286,7 @@ struct sockaddr
>  
>  struct sockaddr_storage
>  {
> -	sa_family_t ss_family;
> +	sa_family_t ss_family, __ss_family_pad;
>  	unsigned long __ss_align;
>  	char __ss_padding[128-2*sizeof(unsigned long)];
>  };

This is wrong for 64-bit archs; new version attached.

Rich

[-- Attachment #2: sockaddr_storage_v2.diff --]
[-- Type: text/plain, Size: 370 bytes --]

diff --git a/include/sys/socket.h b/include/sys/socket.h
index 6788375..c7f244a 100644
--- a/include/sys/socket.h
+++ b/include/sys/socket.h
@@ -287,6 +287,7 @@ struct sockaddr
 struct sockaddr_storage
 {
 	sa_family_t ss_family;
+	char __ss_family_pad[sizeof(long)-sizeof(sa_family_t)];
 	unsigned long __ss_align;
 	char __ss_padding[128-2*sizeof(unsigned long)];
 };

Re: sockaddr_storage and GCC 6.1

[Rich Felker]

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

On Tue, May 24, 2016 at 03:07:35PM -0700, William Ahern wrote:
> GCC 6.1 more aggressively decomposes aggregate assignments into a series of
> scalar member assignments. This has uncovered an issue with glibc's layout
> of struct sockaddr_storage, which has a padding hole from offsets 2 to 8,
> precisely where .sin_port and .sin_addr are in struct sockaddr_in.
> 
>   https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71120
> 
> musl shares this same issue. Specifically, the __ss_align member with an
> 8-byte alignment on LP64 archs. You can track the glibc resolution at
> 
>   https://sourceware.org/bugzilla/show_bug.cgi?id=20111
> 
> Or not track it. Reasonable folks can disagree regarding many aspects of
> this issue, but I thought it worthwhile to bring to people's attention.

I maintain that it's a bug (violation of effective type rules) for a
program to attempt to copy sockaddr types using sockaddr_storage, but
this is a nasty application bug to track down (usually silent
breakage) that's worth avoiding since it's easy. Does the attached
patch work?

I don't think we should even consider the sorts of may_alias hacks
glibc/gcc folks are discussing, though. There's already a gcc option
for compiling broken code like that; it's called -fno-strict-aliasing.

Rich

[-- Attachment #2: sockaddr_storage.diff --]
[-- Type: text/plain, Size: 356 bytes --]

diff --git a/include/sys/socket.h b/include/sys/socket.h
index 6788375..d2bd5df 100644
--- a/include/sys/socket.h
+++ b/include/sys/socket.h
@@ -286,7 +286,7 @@ struct sockaddr
 
 struct sockaddr_storage
 {
-	sa_family_t ss_family;
+	sa_family_t ss_family, __ss_family_pad;
 	unsigned long __ss_align;
 	char __ss_padding[128-2*sizeof(unsigned long)];
 };

sockaddr_storage and GCC 6.1

[William Ahern]

GCC 6.1 more aggressively decomposes aggregate assignments into a series of
scalar member assignments. This has uncovered an issue with glibc's layout
of struct sockaddr_storage, which has a padding hole from offsets 2 to 8,
precisely where .sin_port and .sin_addr are in struct sockaddr_in.

  https://gcc.gnu.org/bugzilla/show_bug.cgi?id=71120

musl shares this same issue. Specifically, the __ss_align member with an
8-byte alignment on LP64 archs. You can track the glibc resolution at

  https://sourceware.org/bugzilla/show_bug.cgi?id=20111

Or not track it. Reasonable folks can disagree regarding many aspects of
this issue, but I thought it worthwhile to bring to people's attention.

- Bill