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

Unsoundness in pleco_engine/src/root_moves/root_moves_list.rs "insert_score_depth" and "insert_score" #163

Open
lwz23 opened this issue Nov 27, 2024 · 0 comments

Comments

@lwz23
Copy link

lwz23 commented Nov 27, 2024

While I scan the crates.io project I notice the following two function:

pub fn insert_score_depth(&mut self, index: usize, score: i32, depth: i16) {

pub fn insert_score(&mut self, index: usize, score: i32) {

#[inline]
    pub fn insert_score_depth(&mut self, index: usize, score: i32, depth: i16) {
        unsafe {
            let rm: &mut RootMove = self.get_unchecked_mut(index);
            rm.score = score;
            rm.depth_reached = depth;
        }
    }

    #[inline]
    pub fn insert_score(&mut self, index: usize, score: i32) {
        unsafe {
            let rm: &mut RootMove = self.get_unchecked_mut(index);
            rm.score = score;
        }
    }

Bug Description:
In pleco_engine, the insert_score_depth and insert_score functions use unsafe operations to access an index and modify fields of RootMove. We have noticed that if an invalid index is passed, it can lead to Undefined Behavior (UB). Specifically, the function get_unchecked_mut does not check the validity of the index, so passing an invalid index may cause memory corruption or crashes.
Steps to Reproduce:

  1. Call insert_score_depth or insert_score with the following code:
let mut root_move_list = RootMoveList::new();  // Assuming this is initialization code
let invalid_index = 1000;  // Assume index 1000 is out of bounds
root_move_list.insert_score_depth(invalid_index, 100, 10);
  1. Or:
    root_move_list.insert_score(invalid_index, 100);
  2. The above code attempts to insert at an invalid index, which leads to unsafe memory access.

Problem Description:
The get_unchecked_mut function accesses memory without checking the validity of the index.
If an invalid index is passed, it may trigger Undefined Behavior (UB), which could lead to memory corruption or a program crash.

Expected Behavior:
The insert_score_depth and insert_score functions should check the validity of the index before accessing data, preventing invalid memory access.
To avoid unsafe memory access, it's recommended to either add boundary checks or use a safer indexing method.

Suggested Fix:
Add logic to check if the index is within bounds before performing the memory access.
Use Rust’s safe API (get_mut) instead of the unsafe get_unchecked_mut, or at least add explicit boundary checks.

#[inline]
pub fn insert_score_depth(&mut self, index: usize, score: i32, depth: i16) {
    if index >= self.len() {  // Add boundary check
        panic!("Index out of bounds");
    }
    unsafe {
        let rm: &mut RootMove = self.get_unchecked_mut(index);
        rm.score = score;
        rm.depth_reached = depth;
    }
}

#[inline]
pub fn insert_score(&mut self, index: usize, score: i32) {
    if index >= self.len() {  // Add boundary check
        panic!("Index out of bounds");
    }
    unsafe {
        let rm: &mut RootMove = self.get_unchecked_mut(index);
        rm.score = score;
    }
}

Additional Information:
Please ensure that performance and safety are balanced when making the fix.
Since these two functions are written with unsafe code, a careful review of the impact on other parts of the code is recommended during the fix.

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

No branches or pull requests

1 participant