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

Make all Nix libc wrapper types repr(transparent)/From<libc::xx>/Into<libc::xx> #2327

Open
SteveLauC opened this issue Mar 3, 2024 · 8 comments

Comments

@SteveLauC
Copy link
Member

SteveLauC commented Mar 3, 2024

As described in this comment, all the Nix wrapper types should be:

  1. #[repr(transparent)]
  2. impl From<libc::xx> for Wrapper
  3. impl From<Wrapper> for libc::xx
@SteveLauC SteveLauC changed the title Make all NIx libc wrapper types repr(transparent)/From<libc::xx> Make all Nix libc wrapper types repr(transparent)/From<libc::xx>/Into<libc::xx> Mar 3, 2024
@SteveLauC
Copy link
Member Author

My current thought on this is that impl From<libc::xx> for Wrapper can be unsafe because we cannot ensure the values set in those libc types are valid

@asomers
Copy link
Member

asomers commented Apr 13, 2024

My current thought on this is that impl From<libc::xx> for Wrapper can be unsafe because we cannot ensure the values set in those libc types are valid

Yes. Especially for cases where the libc type is a union, for example.

@jonatanzeidler
Copy link

jonatanzeidler commented May 6, 2024

There are cases, where the data does not originate from C code, but from Rust code directly. Wouldn't it make sense to have a native Rust type that is guaranteed to be complete as alternative to the libc types? Something like:

    /// A Rust representation of [libc::sockaddr_ll] with the additional
    /// guarantee of valid data, so that this can be made into a [LinkAddr]
    /// without the need of `unsafe` code.
    ///
    /// # Examples
    ///
    /// ```edition2021
    /// use nix::sys::socket::{LinkAddr, SockAddrLl};
    ///
    /// let addr = LinkAddr::from(SockAddrLl {
    ///     sll_family: libc::AF_PACKET as u16,
    ///     sll_ifindex: 0,
    ///     sll_protocol: 0,
    ///     sll_addr: [0; 8],
    ///     sll_halen: 0,
    ///     sll_hatype: 0,
    ///     sll_pkttype: 0,
    /// });
    /// ```
    #[derive(Copy, Clone, Debug)]
    pub struct SockAddrLl {
        /// See [libc::sockaddr_ll::sll_family]
        pub sll_family: u16,
        /// See [libc::sockaddr_ll::sll_protocol]
        pub sll_protocol: u16,
        /// See [libc::sockaddr_ll::sll_ifindex]
        pub sll_ifindex: i32,
        /// See [libc::sockaddr_ll::sll_hatype]
        pub sll_hatype: u16,
        /// See [libc::sockaddr_ll::sll_pkttype]
        pub sll_pkttype: u8,
        /// See [libc::sockaddr_ll::sll_halen]
        pub sll_halen: u8,
        /// See [libc::sockaddr_ll::sll_addr]
        pub sll_addr: [u8; 8]
    }

    impl From<SockAddrLl> for libc::sockaddr_ll {
        fn from(sll: SockAddrLl) -> Self {
            Self {
                sll_family: sll.sll_family,
                sll_protocol: sll.sll_protocol,
                sll_ifindex: sll.sll_ifindex,
                sll_hatype: sll.sll_hatype,
                sll_pkttype: sll.sll_pkttype,
                sll_halen: sll.sll_halen,
                sll_addr: sll.sll_addr,
            }
        }
    }

    impl From<SockAddrLl> for LinkAddr {
        fn from(sll: SockAddrLl) -> Self {
            Self(sll.into())
        }
    }

@asomers
Copy link
Member

asomers commented May 6, 2024

There are cases, where the data does not originate from C code, but from Rust code directly. Wouldn't it make sense to have a native Rust type that is guaranteed to be complete as alternative to the libc types? Something like:

That involves doing data copies during every transition from C to Rust or vice versa. Nix strives for 0-cost abstractions instead. Wherever possible, Nix's Rust accessors should manipulate the raw C struct.

@jonatanzeidler
Copy link

jonatanzeidler commented May 6, 2024

My suggestion does not replace any existing approach, but adds an alternative way for initialization that happens once for a socket. I'd argue that this optional one-time copy is a valid trade-off for getting rid of unsafe code (if the copy doesn't get optimized away anyways). Adding #[inline] to the two from() functions, the compiler seems to actually eliminate the copying in the typical case, using opt level 1. I tried the following example on https://godbolt.org/:

    pub struct LinkAddr(sockaddr_ll);

    #[repr(C)]
    pub struct sockaddr_ll {
        pub sll_family: u16,
        pub sll_protocol: u16,
        pub sll_ifindex: i32,
        pub sll_hatype: u16,
        pub sll_pkttype: u8,
        pub sll_halen: u8,
        pub sll_addr: [u8; 8]
    }

    pub struct SockAddrLl {
        pub sll_family: u16,
        pub sll_protocol: u16,
        pub sll_ifindex: i32,
        pub sll_hatype: u16,
        pub sll_pkttype: u8,
        pub sll_halen: u8,
        pub sll_addr: [u8; 8]
    }

    impl From<SockAddrLl> for sockaddr_ll {
        #[inline]
        fn from(sll: SockAddrLl) -> Self {
            Self {
                sll_family: sll.sll_family,
                sll_protocol: sll.sll_protocol,
                sll_ifindex: sll.sll_ifindex,
                sll_hatype: sll.sll_hatype,
                sll_pkttype: sll.sll_pkttype,
                sll_halen: sll.sll_halen,
                sll_addr: sll.sll_addr,
            }
        }
    }

    impl From<SockAddrLl> for LinkAddr {
        #[inline]
        fn from(sll: SockAddrLl) -> Self {
            Self(sll.into())
        }
    }

pub fn usage(f: u16, p: u16, i: i32, h: u16, pk: u8, ha: u8, a: [u8; 8]) {
    let addr = LinkAddr::from(SockAddrLl {
         sll_family: f,
         sll_ifindex: i,
         sll_protocol: p,
         sll_addr: a,
         sll_halen: ha,
         sll_hatype: h,
         sll_pkttype: pk,
     });
     std::hint::black_box(&addr);
}

@jonatanzeidler
Copy link

jonatanzeidler commented May 6, 2024

In order to get rid of the copy, we could also make SockAddrLl a newtype around libc::sockaddr_ll and let the user move all values into the constructor (hinting to and hoping for in-lining). Would you prefer that?

@SteveLauC
Copy link
Member Author

SteveLauC commented May 6, 2024

In order to get rid of the copy, we could also make SockAddrLl a newtype around libc::sockaddr_ll and let the user move all values into the constructor (hinting to and hoping for in-lining). Would you prefer that?

The constructor way has been discussed in this comment, and:

I don't like the proposed constructors because they're potentially too limited. On Linux and OSX, the libc structure has 7 fields. On the BSDs, it has between 8 and 10. We don't want to define a new constructor for every combination of fields that the user might want to initialize. I think it would be better to either use the Builder pattern, or else rely on getifaddrs to initialize the structure, and then provide mutators as necessary.

@SteveLauC
Copy link
Member Author

Related: #1977

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants