Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SR-2749] Invalid copy of sockaddr_storage struct on Linux #45353

Open
Bouke opened this issue Sep 24, 2016 · 25 comments
Open

[SR-2749] Invalid copy of sockaddr_storage struct on Linux #45353

Bouke opened this issue Sep 24, 2016 · 25 comments
Labels
bug A deviation from expected or documented behavior Compiler

Comments

@Bouke
Copy link
Contributor

Bouke commented Sep 24, 2016

Previous ID SR-2749
Radar rdar://problem/37067884
Original Reporter @Bouke
Type Bug
Environment

Swift 3.0, Swift 3.0.1 Preview 1 on Ubuntu 14.04

Additional Detail from JIRA
Votes 1
Component/s Compiler
Labels Bug
Assignee None
Priority Medium

md5: 49ede49423188b934fa6fdf263d17ba0

Issue Description:

#if os(OSX)
print("NOTE: This bug only demonstrates on Linux")
import Darwin
#elseif os(Linux)
import Glibc
#endif

import struct Foundation.Data

func portFromCopy(_ original: sockaddr_storage) -> UInt16 {
    var ss = original
    return withUnsafeMutablePointer(to: &ss) {
        print("First 16 bytes of the copy:      \(Array(Data(bytes: $0, count: 16)))")
        return $0.withMemoryRebound(to: sockaddr_in.self, capacity: 1) {
            return $0.pointee.sin_port.bigEndian
        }
    }
}

func portFromOriginal(_ ss: inout sockaddr_storage) -> UInt16 {
    return withUnsafeMutablePointer(to: &ss) {
        print("First 16 bytes of the argument:  \(Array(Data(bytes: $0, count: 16)))")
        return $0.withMemoryRebound(to: sockaddr_in.self, capacity: 1) {
            return $0.pointee.sin_port.bigEndian
        }
    }
}

var ss = sockaddr_storage()
withUnsafePointer(to: &ss) {
    $0.withMemoryRebound(to: sockaddr_in.self, capacity: 1) {
        $0.pointee.sin_family = sa_family_t(AF_INET)
        $0.pointee.sin_addr = in_addr(s_addr: 0)
        $0.pointee.sin_port = (38583 as in_port_t).bigEndian
        print("The port is set to:              \($0.pointee.sin_port.bigEndian)")
    }
}

// correct
withUnsafeMutablePointer(to: &ss) {
    $0.withMemoryRebound(to: sockaddr_in.self, capacity: 1) {
        print("Using the variable:              \($0.pointee.sin_port.bigEndian)")
    }
}

// correct
var copy = ss
withUnsafeMutablePointer(to: &copy) {
    $0.withMemoryRebound(to: sockaddr_in.self, capacity: 1) {
        print("Using a copy:                    \($0.pointee.sin_port.bigEndian)")
    }
}

// incorrect
print("Using copy function:             \(portFromCopy(ss))")

// correct
print("Using inout function:            \(portFromOriginal(&ss))")

The code above should print the same port number. However the portFromCopy returns a random port number each time. Could it be that it is looking at a wrong memory address? It should just inspect the mutable copy of the parameter passed in.

@belkadan
Copy link
Contributor

belkadan commented Sep 26, 2016

Hm. @atrick, see anything obviously wrong here?

@atrick
Copy link
Member

atrick commented Sep 26, 2016

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.

@atrick
Copy link
Member

atrick commented Sep 29, 2016

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..

@atrick
Copy link
Member

atrick commented Jul 7, 2017

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

@Lukasa
Copy link
Contributor

Lukasa commented Jan 31, 2018

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

@Lukasa
Copy link
Contributor

Lukasa commented Jan 31, 2018

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") }
    }
}

@Lukasa
Copy link
Contributor

Lukasa commented Jan 31, 2018

@swift-ci create

@Lukasa
Copy link
Contributor

Lukasa commented Jan 31, 2018

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.

@weissi
Copy link
Member

weissi commented Jan 31, 2018

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

@jckarter
Copy link
Member

jckarter commented Feb 1, 2018

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?

@belkadan
Copy link
Contributor

belkadan commented Feb 1, 2018

Can someone post the definition of sockaddr_storage on Ubuntu 14.04?

@Lukasa
Copy link
Contributor

Lukasa commented Feb 2, 2018

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.

@Lukasa
Copy link
Contributor

Lukasa commented Feb 2, 2018

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).

@Lukasa
Copy link
Contributor

Lukasa commented Feb 2, 2018

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.

@Lukasa
Copy link
Contributor

Lukasa commented Feb 2, 2018

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.

@belkadan
Copy link
Contributor

belkadan commented Feb 2, 2018

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

@belkadan
Copy link
Contributor

belkadan commented Feb 2, 2018

Good job tracking this down, Cory!

@Lukasa
Copy link
Contributor

Lukasa commented Feb 2, 2018

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.

@belkadan
Copy link
Contributor

belkadan commented Feb 2, 2018

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.

@rjmccall
Copy link
Member

rjmccall commented Feb 2, 2018

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

@jckarter
Copy link
Member

jckarter commented Feb 2, 2018

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.

@atrick
Copy link
Member

atrick commented Feb 2, 2018

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

@jckarter
Copy link
Member

jckarter commented Feb 2, 2018

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

@rjmccall
Copy link
Member

rjmccall commented Feb 2, 2018

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.

@jckarter
Copy link
Member

jckarter commented Feb 2, 2018

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.)

@swift-ci swift-ci transferred this issue from apple/swift-issues Apr 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A deviation from expected or documented behavior Compiler
Projects
None yet
Development

No branches or pull requests

7 participants