Change Sockaddr to a union

[asomers]

PR #1504 addresses a few safety and soundness issues with our Sockaddr type, which all boil down to "We should never create a &T reference if the entire T isn't initialized". But in reviewing it, I realized another problem: many Nix functions force the user to allocate a struct large enough for any type of sockaddr, even if they know exactly which type they need. For example, nix::sys::socket::bind takes an enum Sockaddr as its argument, which can be very large, even though all it does is immediately transform that argument into a pointer. Many Nix users undoubtedly know which type of Sockaddr they need before they call bind, but we force them to allocate a whole enum Sockaddr. And that begs the question: why use an enum at all? These types are basically already an enum in C. They can all be cast to and from sockaddr and sockaddr_storage, using the s*_family field as a discriminator. So here is my proposal for how to fix the problems raised by #1504 , stop requiring such large allocations, and make the sockaddr types more ergonomic too:

• Create a SockaddrLike trait, mostly because for the as_ffi_pair method. The trait will be sealed, because it requires the sa_family_t and sa_len fields to be located at fixed offsets.
• Create newtypes for every specific sockaddr type, all with #[repr(transparent)].
• Create a Rust union for SockaddrStorage. Thanks to the sa_family_t tag, this union will be safely, falliably convertible to the other sockaddr types.
• Change all functions like bind to accept a generic parameter of any SockaddrLike type.
• Deprecate enum Sockaddr and related functions.

@coolreader18 what do you think of this idea? Here's an outline of the code:

/// Anything that, in C, can be cast back and forth to `sockaddr`.
pub trait SockaddrLike: private::Sealed {
    const FAMILY: AddressFamily;

    // Unsafe constructor from a variable length source
    // Some C APIs from provide `len`, and others do not.  If it's provided it
    // will be validated.  If not, it will be guessed based on the family.
    unsafe from_raw(addr: *const libc::sockaddr, len: Option<libc::socklen_t>)
        -> Result<Self> {...}

    // The next three methods have the same implementation for all concrete
    // types, since the trait is sealed!  This works because all implementors
    // are #[repr(transparent)] and the socklen and sa_family fields are located
    // at the same offset for all structs
    fn family(&self) -> AddressFamily {...}
    fn socklen(&self) -> socklen_t {...}
    // Used for many syscalls
    fn as_ffi_pair(&self) -> (*const libc::sockaddr, libc::socklen_t) {...}
}

mod private {
    pub trait Sealed {}
}

#[repr(transparent)]
pub struct Sockaddr(libc::sockaddr);
impl SockaddrLike for Sockaddr {...}

#[repr(transparent)]
pub struct SockaddrIn(libc::sockaddr_in);
impl SockaddrLike for SockaddrIn {...}

#[repr(C)]
pub union SockaddrStorage {
    sa: Sockaddr,
    sin: SockaddrIn,
    storage: libc::sockaddr_storage,
    // And several other types, too
}
impl SockaddrLike for SockaddrStorage {...}

impl SockaddrStorage {
    // Safe because it validates all fields
    pub fn as_sockaddr_in(&self) -> Result<&SockaddrIn> {...}
}

// Conversions by value from specific to generic are always safe, and will
// usually increase the size of the structure.
impl From<SockaddrIn> for SockaddrStorage {...}

// Conversions from generic to specific are falliable
impl TryFrom<SockaddrStorage> for SockaddrIn {...}

// These functions accept any SockaddrLike input, and do not necessarily require
// allocating any large Enum types.
pub fn bind<T: SockaddrLike>(fd: RawFd, addr: &T) -> Result<()> {...}
pub fn getpeername<T: SockaddrLike>(fd: RawFd) -> Result<T> {...}

[coolreader18]

That seems like a good idea! The one concern I'd have is that it makes it harder to introspect a SockAddr, since you can't match on a union. Also, a union would just make it slightly easier to cast to the desired types, you could just do &self.sockaddr_storage as *const _ as *const sockaddr_foo, and I'd guess the public API would be the same no matter the inner casting strategy used. To me, SockAddr is useful because it allows one to match on the sockaddr family, and making it opaque seems like a dowgrade. Though I agree, the duplicate discriminant in the current enum is sorta dumb, but iirc size_of::<SockAddr>() <= size_of::<sockaddr_storage>() since there's only a certain set of sockaddr types nix supports (with sockaddr_un being the biggest one).

[coolreader18]

But SockaddrLike and a wrapper type for sockaddr_storage do seem like good ideas!

[coolreader18]

Would this be pre or post 0.23?

[asomers]

Yes, you wouldn't be able to match on a Sockaddr. You'd have to do this instead, which is a little bit more complicated, but only a little bit:

match sa.family() {
    AddressFamily::Inet => sa.as_sockaddr_in().unwrap().do_something(),
    ...
}

This would be post 0.23. I don't want to hold up the release any longer, so I would revert #1447 , do the release, then start working on this new scheme.

[asomers]

I've been working on this. But Unix domain sockets are an Achilles' heel. There's simply no way to distinguish an unnamed socket from an abstract socket unless you also know the length of the sun_path field, which isn't recorded anywhere within the structure itself.

[asomers]

Ok, I think I have a plan that will handle Unix-domain sockets:

• Make the SockaddrStorage union contain a UnixAddr member, rather than a bare sockaddr_un. If #[repr(C)], its sun_family field should still line up with the sa_family field.
• The SockaddrLike::socklen method can't have a default implementation, on Linux at least. It must be separately implemented for each struct. On the BSDs it could use a default implementation as an optimization.
• As an additional optimization, UnixAddr could probably omit the path_len field on the BSDs.

[rtzoeller]

@asomers is this closed by #1618?

[asomers]

No, that was just one step. I've got a WIP branch for the rest of the changes.

[rtzoeller]

@asomers once this is resolved, are we ready to release 0.24.0?

I'd like to see #1611 make it into a release soon-ish.

[asomers]

Yes, I think so, and I would too. BTW, here is my WIP for this issue:
master...asomers:sockaddr_union2