recvmmsg may cause UB

[Jan561]

Description

recvmmsg currently mutates state through shared references, which is immediately UB. This is caused by using shared references in the function signature, where mutable references are requried.

Detailed Explanation

Currently, recvmmsg essentially does the following:

pub fn recvmmsg<'a, XS>(
    // elided ...
    slices: XS,
)
where
    XS: IntoIterator<Item = &'a I>,
    I: AsRef<[IoSliceMut<'a>]> + 'a,
{
    for slice in slices.into_iter() {
        let ptr_mut = slice.as_ref().as_ptr() as *mut libc::iovec;

        // Continue using `ptr_mut` in a mutable context
    }
}

This boils down to: (stripping one layer of slices)

pub fn recvmmsg<'a>(
    // elided ...
    slices: &'a [&'a mut [u8]],
) {
    for slice in slices.iter() {
        let ptr = &**slice as *const [u8];
        let ptr_mut = ptr.cast_mut();

        // Continue using `ptr_mut` in a mutable context
    }
}

Although we have a mutable reference to the inner slice, it is hidden behind a shared reference, which behaves exactly like it was borrowed immutably in first place. Casting a shared reference to a *mut _ pointer and mutating the value through that pointer is always UB.

Example demonstrating this bug

use std::{
    io::IoSliceMut,
    net::{SocketAddr, UdpSocket},
    os::fd::AsRawFd,
};

use crossbeam::thread::scope;
use nix::sys::socket::{recvmmsg, MsgFlags, MultiHeaders, SockaddrIn};

fn main() {
    let mut buf = [0u8; 2];
    let i = [IoSliceMut::new(&mut buf[..])];
    let xs = [&i];

    scope(|s| {
        s.spawn(|_| loop {
            let socket = UdpSocket::bind("0.0.0.0:42".parse::<SocketAddr>().unwrap()).unwrap();
            let slices = xs.iter();

            let mut h = MultiHeaders::<SockaddrIn>::preallocate(1, None);

            // This thread is mutating `buf` ...
            recvmmsg(socket.as_raw_fd(), &mut h, slices, MsgFlags::empty(), None).unwrap();
        });

        s.spawn(|_| loop {
            let slices = xs.iter();

            for slice in slices {
                // While this thread tries to read `buf`
                println!("{:?}", slice);
            }
        });
    })
    .unwrap();
}

Proposal

Change the function signature of recvmmsg to:

pub fn recvmmsg<'a, XS, S, I>(
    fd: RawFd,
    data: &'a mut MultiHeaders<S>,
    slices: XS,
    flags: MsgFlags,
    mut timeout: Option<crate::sys::time::TimeSpec>,
) -> crate::Result<MultiResults<'a, S>>
where
    XS: IntoIterator<Item = &'a mut I>,
    I: AsMut<[IoSliceMut<'a>]> + 'a,

or (preferably, but inconsistent with sendmmsg)

pub fn recvmmsg<'a, XS, S, I>(
    fd: RawFd,
    data: &'a mut MultiHeaders<S>,
    slices: XS,
    flags: MsgFlags,
    mut timeout: Option<crate::sys::time::TimeSpec>,
) -> crate::Result<MultiResults<'a, S>>
where
    XS: IntoIterator<Item = I>,
    I: AsMut<[IoSliceMut<'a>]>,

[Jan561]

As a side note, I'm not convinced that RecvMsg containing a reference to the underlying IO slices is a good thing, as this defeats the purpose of using references (and slices) as arguments to this function.

Even worse, RecvMsg only gives the caller a shared reference to the data, which is tied to the lifetime of the IO slice. Until RecvMsg gets dropped, the caller is unable to obtain a mutable reference to the buffer in safe rust and would be forced to either copy the data into a new buffer before modifying it or obtain a mutable reference using unsafe code.

In the context of recvmmsg, the caller first would have to fully exhaust and drop the MultiResults Iterator before the IO slice can be mutably borrowed again.

[asomers]

More than anything else, the recvmsg family of functions makes me want to quit my programming job and start a career cleaning toilets. That's why I've procrastinated on this issue for so long. But I've spent some time today reviewing all this stuff, and I think that you're right. I think there are a bunch of other issues with our sendmsg/recvmsg APIs too, but we can fix them separately.