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

Possible UB since preadv accepts &[IoVec<&mut [u8]>] #1371

Closed
4lDO2 opened this issue Jan 6, 2021 · 2 comments · Fixed by #1855
Closed

Possible UB since preadv accepts &[IoVec<&mut [u8]>] #1371

4lDO2 opened this issue Jan 6, 2021 · 2 comments · Fixed by #1855

Comments

@4lDO2
Copy link

4lDO2 commented Jan 6, 2021

In Rust, attempting to obtain a mutable reference from an immutable reference is immediately Undefined Behavior, unless UnsafeCell is used, which includes mutable references behind immutable references (&&mut [u8], or in this case &[IoVec<&mut [u8]>]). This has also been extended to regular pointers, that originate from stack variables or regular references, now that Stacked Borrows is about to get integrated into the compiler. While the Rust compiler may not be ever able to detect UB within preadv, as syscalls are opaque and could have other side-effects, it could theoretically optimize subsequent code under the assumption that the iovec is aliased, and hence cannot allow its data to be mutated for the lifetime of the system call. Not only that, but this behavior could also change arbitrarily in future compiler versions.

What I suggest we do to fix this potential issue, is to revert the function signature in preadv (changed in #914) to take &mut [IoVec<&mut [u8]>], which is something libstd already does, as well as the regular readv call in sys::uio. Additionally, we will need to change the implementations of readv and preadv to take as_mut_ptr rather than as_ptr, since Rust allows no mutation from any pointer coming from as_ptr (which borrows &self immutably).

@sporksmith
Copy link
Contributor

sporksmith commented May 18, 2021

Came here to file the same issue for process_vm_readv, which has the same problem. IANA expert, but I started digging into this when I was writing a wrapper for process_vm_readv, and found I couldn't have it take &[&mut [u8]] without using unsafe to coerce pointers, and doing so would cause Miri to flag it as UB.

The language docs on interior mutability seem pretty clear that the current implementation is UB. https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#interior-mutability

@sporksmith
Copy link
Contributor

sporksmith commented May 18, 2021

Example in the Rust playground that (I think) implements the same behavior as these methods. It works with unsafe, but doesn't pass the Miri checks. https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=8cb8a4507c42d86f6efd8a81d31f89d6

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