GitLab

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:
  1. 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.
  2. c_string_to_slice then turns it into a &[u8]. This is undefined because & implies the llvm noalias attribute which implies that we have exclusive access to this memory. As before, the user might be modifying the memory.
  3. 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.