Definition of the SockaddrFromRaw trait

[Jan561]

Preparation to fyx #2177.

Example Implementation for SockaddrIn:

unsafe impl private::SockaddrFromRawPriv for Option<SockaddrIn> {
    type Storage = libc::sockaddr_in;

    unsafe fn from_raw(
        addr: *const Self::Storage,
        len: usize,
    ) -> Self {
        if len > mem::size_of::<libc::sockaddr_in>() {
            return None;
        }

        // SAFETY: `sa_family` has been initialized by `Self::init_storage` or by the syscall.
        unsafe {
            if addr_of!((*addr).sin_family).read() as libc::c_int != libc::AF_INET {
                return None;
            }
        }

        unsafe {
            Some(*addr.cast())
        }
    }

    fn init_storage(buf: &mut MaybeUninit<Self::Storage>) {
        // The family of `Self` is `AF_INET`, so setting the family to `AF_UNSPEC` is sufficient.
        let ptr = buf.as_mut_ptr() as *mut libc::sockaddr;
        unsafe { addr_of_mut!((*ptr).sa_family).write(libc::AF_UNSPEC as _) }
    }
}

impl SockaddrFromRaw for Option<SockaddrIn> {}

Alternatives

The functionality of this trait could also be embedded into SockaddrLike. But this would force us to set the return type of the from_raw function to Option<XXX>. This has one disadvantage:

• The conversion to SockaddrStorage can never fail. We can assume that len is always in bounds of libc::sockaddr_storage or otherwise panic.

Checklist:

• I have read CONTRIBUTING.md
• I have written necessary tests and rustdoc comments
• A change log has been added if this PR modifies nix's API

[asomers]

I don't understand what problem this PR would solve. Could you please explain?

[Jan561]

https://man7.org/linux/man-pages/man3/recvmsg.3p.html:

If the socket is connected, the msg_name and msg_namelen members shall be ignored.

That means that we don't get any feedback whether the address buffer got initialized or not.

The only way to ensure it is initialized when returning from recvmsg is to either:

• initialize it before calling recvmsg
• write "bad" bytes into the address buffer. If it got initialized by the syscall, these bytes get overwritten, which can be safely checked.

As discussed in #2177, the current implementation SockaddrLike::from_raw can even return successfully if the address buffer is completely unitialized. This is very problematic for address types like UnixAddr that rely on multiple invariants for safety.

The only alternative I see to this is to check every invariant of the address type after recvmsg. For UnixAddr, that includes checking that the path is valid, is NUL-terminated and it's length is equal to the passed length, which can be quite expensive.

[Jan561]

Basically, the call order of recvmsg would then be

S::init_storage -> syscall -> S::from_raw

[Jan561]

On second thought, I don't think that implementations for MaybeUninit<XXX> would be very beneficial. The only step we can really skip for implementations of MaybeUninit are the checks in from_raw that are very cheap. MaybeUninit<SockaddrStorage> and MaybeUninit<UnixAddr> would still need to zero the entire storage in init_storage (otherwise we would get problems with Hash)

I've also edited the PR message

[asomers]

The only way to ensure it is initialized when returning from recvmsg is to either:

* initialize it before calling `recvmsg`

* write "bad" bytes into the address buffer. If it got initialized by the syscall, these bytes get overwritten, which can be safely checked.

Fixing that will only require a single line in pack_mhdr_to_receive. I don't see why the trait should need to change. Every SockaddrLike implementation has the sa_family field in the same location.

As discussed in #2177, the current implementation SockaddrLike::from_raw can even return successfully if the address buffer is completely unitialized. This is very problematic for address types like UnixAddr that rely on multiple invariants for safety.

So can your proposed alternative.

The only alternative I see to this is to check every invariant of the address type after recvmsg. For UnixAddr, that includes checking that the path is valid, is NUL-terminated and it's length is equal to the passed length, which can be quite expensive.

The SockaddrLike implementors already check their invariants in from_raw. And if there's something that they're missing, we should add it.

[Jan561]

Fixing that will only require a single line in pack_mhdr_to_receive. I don't see why the trait should need to change. Every SockaddrLike implementation has the sa_family field in the same location.

All types except (). I guess we can check if SockaddrLike::as_ptr returns NULL, but this wouldn't be the prettiest solution, especially because SockaddrLike doesn't provide the necessary safety guarantee.

If we go down this route, we should mark the SockaddrLike trait as unsafe with the following reason:

/// # Safety
///
/// If `&self` is convertible to `&libc::sockaddr`, then`Self::as_ptr` must perform this conversion.
///
/// If `&self` is *not* convertible to `&libc::sockaddr`, then `Self::as_ptr` must return NULL.

So can your proposed alternative.

The difference between my from_raw and the already existing one is the weaker safety invariant that prevents this bug. SockaddrLike::from_raw requires that addr is "valid for the specific type of sockaddr" and we are violating this contract in recvmsg.

[Jan561]

Another difference is that when using S = SockaddrStorage with recvmsg, the version with pack_mhdr_to_receive can still return uninitialized memory (the only thing guaranteed is ss_family = AF_UNSPEC), while with my implementation we can zero everything out in init_buf and always return a defined value. I don't know how high you value this aspect, since you don't consider uninitialized memory as UB in FFI context (which is fine, I understand your reasoning).

I'll open a separate PR if the version with pack_mhdr_to_receive is your preferred option. This PR can be closed in that case.