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

Concerns about this crate #156

Open
Speak2Erase opened this issue Aug 8, 2023 · 0 comments
Open

Concerns about this crate #156

Speak2Erase opened this issue Aug 8, 2023 · 0 comments

Comments

@Speak2Erase
Copy link

I'm currently looking around for a chess AI written in rust. I found Pleco, and it seems like a neat project, it's only one of a handful of crates I've seen that actually implement a chess AI.

However- I have a lot of concerns about this crate.
For starters, the history. @chase-manning made a new version of the crate (Tanton, see #140), but then shortly after seems to be the owner of Pleco. All they've done is various updates to CI and fixing warnings, which is fair enough- but nothing really substantial.

Then, there's the code.
Oh boy, there's a lot of undefined behavior and unsafe code. It's honestly quite daunting to look over it all- so specifically, I'll focus on src/movepick/mod.rs, as it's generally pretty simple.
Let's look at the struct definition for MovePicker:

pub struct MovePicker {
    pick: Pick,
    board: *const Board,
    moves: ScoringMoveList,
    depth: i16,
    ttm: BitMove,
    killers: [BitMove; 2],
    cm: BitMove,
    recapture_sq: SQ,
    threshold: i32,
    main_hist: *const ButterflyHistory,
    capture_hist: *const CapturePieceToHistory,
    cont_hist: *const [*const PieceToHistory; 4],
    cur_ptr: *mut ScoringMove,
    end_ptr: *mut ScoringMove,
    end_bad_captures: *mut ScoringMove,
}

That's.. a lot of pointers. I can't comment on why they're there, but I can speculate.
In MovePicker::new, there are a bunch of references passed as arguments:

    pub fn main_search(
        board: &Board,
        depth: i16,
        main_hist: &ButterflyHistory,
        // there are a lot more
    ) -> Self

..and all of them get stored as a pointer. If I had to guess, whoever wrote the code for MovePicker did not want to deal with lifetimes. Or is a C programmer who uses Rust as if it were C.
At the least this function should be unsafe. What happens if someone passes a Board that goes out of scope?
Then there's this (also in new):

        MovePicker {
            pick,
            board: &*board,
            // more fields
            recapture_sq: unsafe { mem::MaybeUninit::uninit().assume_init() },
            threshold: unsafe { mem::MaybeUninit::uninit().assume_init() },
            /// more fields
        }

Those mem::MaybeUninit::uninit().assume_init()? Insta UB. There's a reason the compiler doesn't let you use uninitialized values- when you create a value on the stack and don't initialize it, the function call will be at the deepest the stack has ever been at best, or point to some random garbage that used to be there. That's assuming that rustc will behave like a C compiler or not put variables into registers. In any case, these can create invalid values (imagine a bool that is not true or false) which can cause a lot of problems.
Further down in the code, there's a similar thing with mem::zeroed!

No comments about why these may be safe by the way!
All of this kind of code is repeated throughout the codebase- it really calls into question the mindset of the person who wrote it.

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