Redox lacks abstractions, causing undefined behavior and an overuse of `unsafe`
Created by: mahkoh
While looking at the code, I've noticed several places where the code seems to be somewhat ad hoc and lacking in abstractions. In some places this causes undefined behavior and it certainly causes more unsafe code to be written than might be strictly necessary. I'm not sure if you're unaware of this or if you've decided that those things are not important right now. However, it might still be useful to write this down at least once.
Let me give an example:
pub unsafe fn do_sys_open(path: *const u8, flags: usize) -> usize {
let mut fd = usize::MAX; // (1)
let reenable: = scheduler::start_no_ints(); // (2)
if let Some(current) = Context::current() {
let path_string =
current.canonicalize(str::from_utf8_unchecked(c_string_to_slice(path))); // (3)
scheduler::end_no_ints(reenable); // (4)
let resource_option = (*::session_ptr).open(&Url::from_string(path_string), flags);
scheduler::start_no_ints();
if let Some(resource) = resource_option {
fd = current.next_fd();
(*current.files.get()).push(ContextFile {
fd: fd,
resource: resource,
});
}
}
scheduler::end_no_ints(reenable);
fd
}
- (1): The first thing we see is that the return value is declared right at the top of the function. This reminds me of the way things are done in the linux kernel, but C does not have RAII, so premature return statements are not as easily possible there. In rust there should be no reason to do this.
- (2): And this is the reason.
start_no_ints
returns a boolean that has to be used to manually reenable the interrupts at the end of the function. The function must not return prematurely. - (3): This causes at least three different kinds of undefined behavior:
-
c_string_to_slice
calculates the length of the user string by dereferencing it. This is racy because other user threads might currently be modifying said memory. Race conditions such as this are undefined behavior in LLVM. The only safe way to dereference user pointers is with atomic operations or in assembly. -
c_string_to_slice
then turns it into a&[u8]
. This is undefined because&
implies the llvmnoalias
attribute which implies that we have exclusive access to this memory. As before, the user might be modifying the memory. - Then it is transmuted to a
&str
even though it might not contain valid UTF8. This is also undefined behavior.
-
- (3) cont.: The user memory is then continuously dereferenced by various functions. As before, this is UB. Naturally this is only true if Redox runs on multiple cpus (cpu cores). But even if it does not right now, this should be done properly from the start.
- (4): The interrupts are then manually re-enabled for a period of time. As before, this should be done in a more clever way to that one cannot forget to disable them again.
Now let me show how I would rewrite this code. I'll be using the iovec
version of open
proposed in the other issue.
// A macro that turns `Result` into a syscall return value.
macro_rules! k2u_try {
($res:expr) => {
match $res {
Ok(v) => v as isize,
Err(e) => -(e.0 as isize),
}
}
};
pub unsafe fn do_sys_open(path: ConstUserPtr<iovec>, flags: usize) -> isize {
k2u_try!(do_sys_open_inner(path, flags))
}
unsafe fn do_sys_open_inner(path: ConstUserPtr<iovec>,
flags: usize) -> Result<i32, Errno> {
let int_guard = scheduler::start_no_ints();
// ^ RAII guard. can re-enable interrupts in its destructor
let context = match Context::current() {
Some(c) => c,
_ => return Err(errno::NoContext), // thanks to RAII
};
let path: iovec = try!(path.copy()); // safely copies the user iovec. fails if the
// pointer is invalid.
if path.size > PATH_MAX { // sanity check
return Err(errno::NoMemory);
}
let mut path = try!(path.copy()); // safely copies the user slice. this can fail
// because of out-of-memory or invalid pointers.
// allocates said memory
try!(current.canonicalize(&mut path)); // might swap out the memory use by path
let resource = try!(int_guard.with_ints(|| (*::session_ptr).open(path, flags)));
// ^ runs the closure with interrupts (possibly) reenabled.
// returns the return value of the closure.
let fd = current.next_fd();
(*current.files.get()).push(ContextFile {
fd: fd,
resource: resource,
});
Ok(fd)
}
I've used try
several times in places where the code previously returned Option
. I believe all of those functions should be rewritten to return Result
to make use of try
.