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

array_queue pop_back should be indexed with index method. #2

Open
ammaraskar opened this issue Sep 26, 2020 · 1 comment
Open

array_queue pop_back should be indexed with index method. #2

ammaraskar opened this issue Sep 26, 2020 · 1 comment

Comments

@ammaraskar
Copy link

In the pop_back function, self.length - 1 is used as the index:

let x = replace(&mut self.array.as_mut()[self.length - 1], unsafe {

This means that after you perform a pop_front there is the potential to read from uninitialized or previously dropped memory. This can lead to a double-drop and an arbitrary read primitive from safe rust. Here's a program that exhibits the read behavior:

#![forbid(unsafe_code)]

use array_queue::ArrayQueue;

fn main() {
    {
        // 1. Allows reading of uninitialized memory.
        //
        // A queue of size 3 is setup like this:
        //     [x, x, x]    length = 0
        //      ^           start
        // where x is uninitialized memory.
        //
        // push_back(a); push_back(b); push_back(c)
        //     [a, b, c]    length = 3
        //      ^           start
        //
        // pop_front(); pop_back():
        //     [x, b, x]    length = 1
        //         ^        start
        //
        // At this point when performing a pop_back, the queue should use the
        // `ArrayQueue::index` method to index into the array properly but
        // instead simply uses `self.length - 1` causing it to read the first
        // x.
        // https://github.com/raviqqe/array-queue/blob/32fa10f8f15140fb64a4cf36a2a834f876c91056/src/array_queue.rs#L98
        let mut a: [u64; 32] = [0x41; 32];
        let mut x: ArrayQueue<[[u64; 32]; 3]> = ArrayQueue::new();
        x.push_back(&&a);
        x.push_back(&&a);
        x.push_back(&&a);

        x.pop_front().unwrap();
        x.pop_back().unwrap();

        let popped = x.pop_back().unwrap();
        println!("Contents of array: {:?}", popped);
        assert_eq!(popped[0], 0x41);
    }

    {
        // 2. Initializes memory with mem::uninitialized, this is instantly
        //    UB for types that cannot inhabit uninitialized. Should be
        //    changed over to MaybeUninit. (Triggers a panic on latest Rust).

        //let mut x: ArrayQueue<[Box<i32>; 3]> = ArrayQueue::new();
        //x.push_back(&Box::new(1));
    }
}

The second part is a bit harder to fix. Rust now has a type called MaybeUninit that is used to safely initialize types in uninitialized memory. I'd recommend at least fixing the index problem in pop_back.

@hxuhack
Copy link

hxuhack commented Jan 18, 2021

@ammaraskar
I cannot tell why using self.length - 1 would cause it to read the first x since the start position now points to `b'. Can you double check? Thanks!

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

2 participants