Skip to content

Comments

Fix unsound unchecked indexing in RootMoveList and Stack::offset#167

Merged
chase-manning merged 2 commits intomainfrom
copilot/fix-unsoundness-in-insert-score
Feb 22, 2026
Merged

Fix unsound unchecked indexing in RootMoveList and Stack::offset#167
chase-manning merged 2 commits intomainfrom
copilot/fix-unsoundness-in-insert-score

Conversation

Copy link
Contributor

Copilot AI commented Feb 22, 2026

insert_score_depth and insert_score use get_unchecked_mut without bounds validation, allowing UB on out-of-bounds index. Stack::offset performs raw pointer arithmetic with no safety checks.

Changes

  • RootMoveList::insert_score_depth / insert_score: Replace unsafe { self.get_unchecked_mut(index) } with safe &mut self[index], leveraging the existing IndexMut impl which bounds-checks through slice indexing
  • Stack::offset: Add debug_assert! on offset magnitude and document safety invariants for callers
  • Add #[should_panic] tests for out-of-bounds access on both methods
// Before: UB on invalid index
let rm: &mut RootMove = self.get_unchecked_mut(index);

// After: panics on invalid index
let rm: &mut RootMove = &mut self[index];
Original prompt

This section details on the original issue you should resolve

<issue_title>Unsoundness in pleco_engine/src/root_moves/root_moves_list.rs "insert_score_depth" and "insert_score"</issue_title>
<issue_description>While I scan the crates.io project I notice the following function:

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

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

pub fn offset(&mut self, count: isize) -> &mut Stack {

#[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;
        }
    }

pub fn offset(&mut self, count: isize) -> &mut Stack {
        unsafe {
            let ptr: *mut Stack = self as *mut Stack;
            &mut *ptr.offset(count)
        }
    }

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.</issue_description>

Comments on the Issue (you are @copilot in this section)


🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.

- Replace unsafe get_unchecked_mut with safe indexing in insert_score_depth
  and insert_score, which provides automatic bounds checking via the
  existing IndexMut trait implementation
- Add debug_assert bounds check and safety documentation to Stack::offset
- Add tests verifying out-of-bounds panics for insert_score_depth and
  insert_score

Co-authored-by: chase-manning <53957795+chase-manning@users.noreply.github.com>
Copilot AI changed the title [WIP] Fix unsoundness in insert_score_depth and insert_score functions Fix unsound unchecked indexing in RootMoveList and Stack::offset Feb 22, 2026
Copilot AI requested a review from chase-manning February 22, 2026 12:33
@chase-manning chase-manning marked this pull request as ready for review February 22, 2026 23:33
@chase-manning chase-manning merged commit 9dc3255 into main Feb 22, 2026
4 checks passed
@chase-manning chase-manning deleted the copilot/fix-unsoundness-in-insert-score branch February 22, 2026 23:33
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 this pull request may close these issues.

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

2 participants