Xudong-Huang / may

rust stackful coroutine library
Apache License 2.0
1.97k stars 83 forks source link

Unsound Use of get_unchecked in wakeup Function #112

Open lwz23 opened 4 days ago

lwz23 commented 4 days ago

Description The wakeup function uses unsafe { self.vec.get_unchecked(id) } to access an element in the self.vec collection without bounds checking. This introduces undefined behavior (UB) if the provided id index is out of bounds. The lack of validation makes this function unsound. https://github.com/Xudong-Huang/may/blob/a66d8fe2ea3c50fbcf3723f26652ce74d7d08378/src/io/sys/unix/epoll.rs#L155

pub fn wakeup(&self, id: usize) {
        let buf = 1u64.to_le_bytes();
        let ret = write(&unsafe { self.vec.get_unchecked(id) }.evfd, &buf);
        trace!("wakeup id={:?}, ret={:?}", id, ret);
    }

Problems: this function is a pub function, so I assume user can control the id field, it cause some problems.

  1. Unsafe Slice Access: The function uses get_unchecked to access self.vec without verifying that the index id is within bounds. If id is out of bounds, undefined behavior occurs immediately.
  2. No Safety Contract: The function is not marked as unsafe, yet it relies on the caller to ensure that the index is valid. This violates Rust’s safety principles, as it hides the safety invariants that must be upheld.
  3. Potential Exploitation: If id is derived from untrusted input or incorrect logic, it could result in out-of-bounds access, leading to memory corruption, crashes, or other unpredictable behavior.

Suggestion

  1. mark this function as unsafe and provide safety doc.
  2. add some check in the function body.

Additional Context: Rust's unsafe constructs require strict validation to maintain safety guarantees. The current implementation of wakeup assumes the index is always valid but does not enforce or document this requirement, making the function unsound. By using safe indexing and providing error handling, the function can prevent undefined behavior while remaining robust and user-friendly.

lwz23 commented 4 days ago

same for https://github.com/Xudong-Huang/may/blob/a66d8fe2ea3c50fbcf3723f26652ce74d7d08378/src/io/sys/windows/iocp.rs#L112 fn select and https://github.com/Xudong-Huang/may/blob/a66d8fe2ea3c50fbcf3723f26652ce74d7d08378/src/scheduler.rs#L133 fn run_queued_tasks and https://github.com/Xudong-Huang/may/blob/a66d8fe2ea3c50fbcf3723f26652ce74d7d08378/src/scheduler.rs#L204 fn schedule_with_id

lwz23 commented 2 days ago

I note that both the io and schedule modules are private, and it seems unlikely that all users will be able to call these potentially problematic functions directly, but since this is the case, I suggest that instead of declaring these functions as pub, it might be more appropriate to declare them as pub(crate).

Xudong-Huang commented 2 days ago

You are correct, thanks!