Unsound Implementation of get_mut in Orderbook Allows Undefined Behavior

[lwz23]

Description
The get_mut function in the Orderbook implementation uses unsafe code to call Buffer::get_unchecked_mut, bypassing bounds checking. This function is unsound because it allows access to an arbitrary index without ensuring that the index is within bounds. If the caller provides an out-of-bounds index, the program will invoke undefined behavior (UB).

ninjabook/src/fixed_orderbook.rs

Line 307 in 7f3b7d3

unsafe { self.buf.get_unchecked_mut(index) }

pub fn get_mut(&mut self, index: usize) -> &mut Level {
        unsafe { self.buf.get_unchecked_mut(index) }
    }

Problems:
Unsafe Access Without Validation:
The function uses unsafe to call get_unchecked_mut without verifying that index is within bounds. This shifts the responsibility of ensuring safety to the caller, violating Rust's safety guarantees.
No Documentation or Safety Contract:
There is no documentation specifying the requirements for the index parameter. The caller is not informed that the index must be valid, leading to potential misuse.
Potential Undefined Behavior:
If index is out of bounds, accessing the buffer through get_unchecked_mut will cause undefined behavior. This could lead to memory corruption, crashes, or other unpredictable outcomes.

Suggestion

1. mark this function as unsafe and comment a doc about safety precondition.
2. add assert in the function body.
   Additional Context
   Rust's unsafe blocks are a powerful tool but must be used with extreme care. By bypassing bounds checks in get_mut, the current implementation sacrifices safety for performance.

[lwz23]

same problem for

ninjabook/src/fixed_orderbook.rs

Line 303 in 7f3b7d3

unsafe { self.buf.get_unchecked(index) }

[lwz23]

here is my PoC:

extern crate ninjabook;

use ninjabook::fixed_orderbook::Buffer;

fn main() {
    let mut a=Buffer::new(true);
    let _b= a.get_mut(9999);
}

Result:

PS E:\Github\lwz> cargo run
   Compiling lwz v0.1.0 (E:\Github\lwz)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.25s
     Running `target\debug\lwz.exe`
thread 'main' panicked at core\src\panicking.rs:223:5:
unsafe precondition(s) violated: slice::get_unchecked_mut requires that the index is within the slice
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
thread caused non-unwinding panic. aborting.
error: process didn't exit successfully: `target\debug\lwz.exe` (exit code: 0xc0000409, STATUS_STACK_BUFFER_OVERRUN)

Considering this is a Unsoundness problem and published to crates.io I suggest report it to RustSec.

[ninja-quant]

Hello, thank you for noticing. However, that orderbook's only use is to run benchmarks, as stated in the file src/fixed_orderbook.rs, lines 5 and 6 read:

/// Implementation of an orderbook with fixed size to use as a benchmark
/// This has no use other than benchmarking.

Just like the src/naive_orderbook.rs, in lines 3 and 4 respectively.

Feel free to code a nicer version that can beat benchmarks against src/orderbook.rs which is the one that can be used in production environment. I will happily review that PR.

In the meantime i will close this Issue, thanks again.