Hm. @atrick, see anything obviously wrong here?

This is a great test case. Nothing is wrong with the source code. Does the bug depend on optimization level?

I don't have a working linux build yet, so can't say anything more at the moment.

The bug reproduces without optimization. With optimization the inlined "var copy = ss" case has the bug as the portFromCopy() case.

It's as-if the lifetime of the inout argument is not being extended to the end of the closure! Maybe there's some stack slot reuse or memory clobbering that only shows up on linux.

I haven't had time to debug beyond that yet..

If anyone watching has a linux build, please try to reproduce this on Swift 4.

I bumped into this today so I can answer @atrick's question: yes, this reproduces on Swift 4.0.2.

So here's a new, smaller repro that demonstrates the same problem (requires an optimised build, though more complex repros can encounter this in debug builds too):

import Glibc

var storage = sockaddr_storage()

withUnsafeMutableBytes(of: &storage) {
    for i in $0.indices {
    $0[i] = UInt8(exactly: i)!
    }
}

var storageCopy = storage

withUnsafeBytes(of: &storage) { originalPtr in
    withUnsafeBytes(of: &storageCopy) { copyPtr in
    let originalData = [UInt8](originalPtr)
    let copyData = [UInt8](copyPtr)
    if originalData != copyData { fatalError("copy doesnt match original") }
    }
}

@swift-ci create

For what it's worth, it seems like the only thing that safely gets copied when you do this is the first two-byte word. I think this is meaningful, as the first two-byte word is the only type on sockaddr_storage that doesn't do something insane.

just reproed this successfully on Ubuntu 14.04. This does not seem to happen on 16.04.

Sounds like it might be a Clang importer issue, as if we're importing the type with the wrong layout. Did the definition in the headers change between 14.04 and 16.04?

Can someone post the definition of sockaddr_storage on Ubuntu 14.04?

Sure. From the latest 14.04 docker image, here's the definition from /usr/include/x86_64-linux-gnu/bits/socket.h:

/* Structure large enough to hold any socket address (with the historical
   exception of AF_UNIX).  We reserve 128 bytes.  */
#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];
  };

We also need the definition of the __SOCKADDR_COMMON macro from /usr/include/x86_64-linux-gnu/bits/sockaddr.h:

/* POSIX.1g specifies this type name for the `sa_family' member.  */
typedef unsigned short int sa_family_t;

#define __SOCKADDR_COMMON(sa_prefix) \
  sa_family_t sa_prefix##family

Acting like a human preprocessor, the definition becomes:

struct sockaddr_storage
  {
    sa_family_t ss_family;    /* Address family, etc.  */
    unsigned long int __ss_align;  /* Force desired alignment.  */
    char __ss_padding[128 - (2 * sizeof (__ss_aligntype))];
  };

To round this out, I can tell you that sa_family_t is a typedef of unsigned short int.

While I'm dockering things, we may as well compare against 16.04, which appears to not have this issue. Here, sockaddr_storage is defined like this (squishing all the definitions into one code block):

#define __ss_aligntype  unsigned long int
#define _SS_PADSIZE \
  (_SS_SIZE - __SOCKADDR_COMMON_SIZE - sizeof (__ss_aligntype))

/* 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 128

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

Here there are two major changes that affect the interpretation of this struct. Firstly, the padding and alignment fields have been swapped around: the padding is now the second element instead of the first. Secondly, the length of the char array has changed to be 128 - sizeof (unsigned short int) - sizeof (unsigned long int).

This rearrangement suggests the problem is going to be padding related.

In both cases, ss_family has a two byte alignment requirement. However, what happens next is different in each case. According to the clang that is packaged for Ubuntu 14.04, sizeof(unsigned long int) is 8 bytes. That means that we will have 6 bytes of padding between ss_family and __ss_align on Ubuntu 14.04, but no bytes of padding between ss_family and ss_padding on 16.04.

To convince myself this was the case, I wrote the following test program:

#include <stdio.h>
#include <sys/socket.h>

int main(int argc, char **argv) {
    struct sockaddr_storage st = {0};
    printf("sizeof sockaddr_storage: %ld\n", sizeof(struct sockaddr_storage));
    printf("address of ss_family: %p, size: %ld\n", &st.ss_family, sizeof(st.ss_family));
    printf("address of __ss_align: %p, size: %ld\n", &st.__ss_align, sizeof(st.__ss_align));
    printf("address of __ss_padding: %p, size: %ld\n", &st.__ss_padding, sizeof(st.__ss_padding));
    return 0;
}

The output on 14.04:

sizeof sockaddr_storage: 128
address of ss_family: 0x7ffd680bde90, size: 2
address of __ss_align: 0x7ffd680bde98, size: 8
address of __ss_padding: 0x7ffd680bdea0, size: 112

On 16.04:

sizeof sockaddr_storage: 128
address of ss_family: 0x7fff2783d9b0, size: 2
address of __ss_align: 0x7fff2783da28, size: 8
address of __ss_padding: 0x7fff2783d9b2, size: 118

Both systems agree on the size, but they disagree on how the structure is laid out. On 16.04 all 128 bytes of the structure are directly addressable: 2 bytes in ss_family, 8 in __ss_align, and 118 in __ss_padding. On 14.04 that's not true: we have 6 bytes of compiler-added padding to force the alignment requirement on __ss_align.

We have a winner. This is definitely padding-related.

I tweaked my repro program to look like this:

import Glibc

var x = sockaddr_storage()
withUnsafeMutableBytes(of: &x) {
    for index in $0.indices {
        $0[index] = UInt8(exactly: index)!
    }
}

var y = x

withUnsafeBytes(of: &x) {
    print([UInt8]($0))
}
withUnsafeBytes(of: &y) {
    print([UInt8]($0))
}

The output of this program is:

[0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127]
[0, 1, 0, 0, 0, 0, 0, 0, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72, 73, 74, 75, 76, 77, 78, 79, 80, 81, 82, 83, 84, 85, 86, 87, 88, 89, 90, 91, 92, 93, 94, 95, 96, 97, 98, 99, 100, 101, 102, 103, 104, 105, 106, 107, 108, 109, 110, 111, 112, 113, 114, 115, 116, 117, 118, 119, 120, 121, 122, 123, 124, 125, 126, 127]

As you can see, the 6 bytes of compiler-added padding are not copied by Swift. This leads to problems when the sockaddr_storage is being used to contain a smaller sockaddr type. For most structs this isn't an issue, but it matters a lot for this struct here because that's literally its only job: if Swift copies this struct for any reason it'll quietly corrupt it.

I have to admit to being a bit surprised to find that Swift does element-wise copies of structures rather than just attempting to copy the entire structure across (as a 128-byte structure this could be done with 16 movups instructions).

Regardless, this behaviour is definitely the source of the bug. I'll leave it up to the compiler folks to decide if and how they want to change this.

Hm. That sounds like something we wouldn't necessarily fix. :-( The C standard doesn't specify the contents of padding bytes, and so Clang could also be doing this.

cc more advanced language lawyer @rjmccall

Good job tracking this down, Cory!

Thanks. 🙂

I have to admit that when I worked out what this was I also had conflicting feelings about whether it should be fixed. It seems inarguable that the libc definition from 14.04 is just bad, which is presumably why it was changed in the interim. However, it's also a part of a platform that Swift nominally supports, and if the plan is to continue to do so then we should try to come up with a plan to fix it.

Here's a possibility: could Swift ship with apinotes for this structure that redefine it using its 16.04 definition (potentially only on Ubuntu if we're worried about destroying the slightly-different Darwin definition)? Accessing its dunder-prefixed members for any reason is completely valueless, so having them point at the right place is unlikely to help. And if it can coerce the Swift compiler into executing the copy properly, it's likely to be the lowest-overhead approach to silently fixing this up.

Yeah, I guess it's too strong to say we wouldn't do something to support sockaddr_storage. Things are just bad here.

API notes doesn't currently have a way to redefine members of structs. I think rather than that we'd just special-case this one struct, since this is not such a common C pattern anyway.

Special-casing this struct to guarantee to copy its padding bytes would be possible if that's the only reasonable option.

How awful would it be if, instead of special-casing this one struct, we copied all imported structs as if by memcpy, to avoid problems with undefined padding altogether? In the wide world of awful C out there, this is probably far from the only place this happens.

As Cory pointed out earlier, trivial structs should be memcpy'd if we care about performance.

Exactly, aside from C rules-lawyering I don't see a great reason to preserve C behavior here if we can easily do better.

I do not think we want to start making semantic guarantees about padding in C structs. If nothing else, I know people are working on optimizations like this in Clang; it seems absurd for us to be more conservative about C than a C compiler.

Also, I'm sure if you asked IRGen to do a copy it would use a memcpy here. The problem is that it's almost certainly being asked to do loads and stores.

Well, Swift is somewhat more likely to expose these kinds of issues since more semantic copies happen than in idiomatic C code in these sorts of situations. (Never mind that the Perfect Swift Optimizer would eventually eliminate most of these copies.)