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

assume_init on partially initizalized socket addresses #2054

Open
stevenengler opened this issue Jun 6, 2023 · 1 comment
Open

assume_init on partially initizalized socket addresses #2054

stevenengler opened this issue Jun 6, 2023 · 1 comment

Comments

@stevenengler
Copy link
Contributor

There are a few places in the code that call assume_init() on a MaybeUninit that may only be partially initialized, which is UB.

nix/src/sys/socket/mod.rs

Lines 2217 to 2242 in 89b4976

pub fn recvfrom<T: SockaddrLike>(
sockfd: RawFd,
buf: &mut [u8],
) -> Result<(usize, Option<T>)> {
unsafe {
let mut addr = mem::MaybeUninit::<T>::uninit();
let mut len = mem::size_of_val(&addr) as socklen_t;
let ret = Errno::result(libc::recvfrom(
sockfd,
buf.as_ptr() as *mut c_void,
buf.len() as size_t,
0,
addr.as_mut_ptr() as *mut sockaddr,
&mut len as *mut socklen_t,
))? as usize;
Ok((
ret,
T::from_raw(
addr.assume_init().as_ptr() as *const sockaddr,
Some(len),
),
))
}
}

nix/src/sys/socket/mod.rs

Lines 2340 to 2369 in 89b4976

pub fn getpeername<T: SockaddrLike>(fd: RawFd) -> Result<T> {
unsafe {
let mut addr = mem::MaybeUninit::<T>::uninit();
let mut len = T::size();
let ret =
libc::getpeername(fd, addr.as_mut_ptr() as *mut sockaddr, &mut len);
Errno::result(ret)?;
T::from_raw(addr.assume_init().as_ptr(), Some(len)).ok_or(Errno::EINVAL)
}
}
/// Get the current address to which the socket `fd` is bound.
///
/// [Further reading](https://pubs.opengroup.org/onlinepubs/9699919799/functions/getsockname.html)
pub fn getsockname<T: SockaddrLike>(fd: RawFd) -> Result<T> {
unsafe {
let mut addr = mem::MaybeUninit::<T>::uninit();
let mut len = T::size();
let ret =
libc::getsockname(fd, addr.as_mut_ptr() as *mut sockaddr, &mut len);
Errno::result(ret)?;
T::from_raw(addr.assume_init().as_ptr(), Some(len)).ok_or(Errno::EINVAL)
}
}

nix/src/sys/socket/mod.rs

Lines 2045 to 2065 in 89b4976

pub fn recvmsg<'a, 'outer, 'inner, S>(fd: RawFd, iov: &'outer mut [IoSliceMut<'inner>],
mut cmsg_buffer: Option<&'a mut Vec<u8>>,
flags: MsgFlags) -> Result<RecvMsg<'a, 'inner, S>>
where S: SockaddrLike + 'a,
'inner: 'outer
{
let mut address = mem::MaybeUninit::uninit();
let (msg_control, msg_controllen) = cmsg_buffer.as_mut()
.map(|v| (v.as_mut_ptr(), v.capacity()))
.unwrap_or((ptr::null_mut(), 0));
let mut mhdr = unsafe {
pack_mhdr_to_receive(iov.as_ref().as_ptr(), iov.len(), msg_control, msg_controllen, address.as_mut_ptr())
};
let ret = unsafe { libc::recvmsg(fd, &mut mhdr, flags.bits()) };
let r = Errno::result(ret)?;
Ok(unsafe { read_mhdr(mhdr, r, msg_controllen, address.assume_init()) })
}

The calling code might call these functions with a S: SockaddrLike larger than the number of bytes the kernel will write during the syscall. For example if a SockaddrStorage is provided to a getsockname() call for an inet socket, the syscall will write a sockaddr_in which is much smaller than the SockaddrStorage, leaving some of the SockaddrStorage bytes uninitialized. Those uninitialized bytes will then be "assumed init" by the assume_init() call, which is UB.

The simplest solution would be to use mem::MaybeUninit::<T>::zeroed() instead since I think all socket address types are POD, so zero values should be allowed. You could also instead use as_ptr() directly on the MaybeUninit without using assume_init() first, but this will give you a *const S instead of a *const sockaddr, but there's no guarantee that a *const S can be cast to a *const sockaddr.

@asomers
Copy link
Member

asomers commented Jun 28, 2023

I thought that uninitialized memory would only trigger UB when the compiler could prove that the memory is uninitialized. But in this case, since the structure is initialized via the C library, that can't happen. Or to put it another way, why isn't it UB to use assume_init after libc::getsockname, even if the OS did initialize the entire structure?

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

Successfully merging a pull request may close this issue.

2 participants