Implementing `array_iterator::Array` with safe Rust can trigger segfault

Hello 🦀 , we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

array_iterator::Array trait is a public & safe trait, but it is possible to implement the trait with safe Rust to trigger undefined behavior in safe Rust.

Proof of Concept

In the program below, the implementation of Array::into_maybeunint() appends uninitialized MaybeUninit<String>s to the end of the iterator. Since ArrayIterator interprets all items within itself to be initialized objects, the for-loop in the main function accesses uninitialized memory as if it is a valid String.

use std::mem::MaybeUninit;

use array_iterator::{Array, ArrayIterator};

struct MyArr(Vec<String>);
impl Array<String> for MyArr {
    type MaybeUninit = Vec<MaybeUninit<String>>;
    
    fn into_maybeunint(self) -> Self::MaybeUninit {
        debug_assert_eq!(std::mem::size_of::<Self::MaybeUninit>(), std::mem::size_of::<Self>());

        let mut partial: Self::MaybeUninit = self.0.into_iter().map(|x| MaybeUninit::new(x)).collect();

        // DANGEROUS! Appending uninitialized objects to iterator..
        for _ in 0..10 {
            partial.push(MaybeUninit::uninit());
        }
        //
        partial
    }
}

fn main() {
    let my_arr = MyArr(vec![
        String::from("Hello"),
        String::from("World")
    ]);
    for x in ArrayIterator::new(my_arr) {
        println!("{} {:?}, {}", x.len(), x.as_bytes(), x);
    }

    panic!(
        "In DEBUG mode,\n
        this panic was unreachable when tested with rustc 1.48.0 on Ubuntu 18.04"
    );
}

When compiled in DEBUG mode with rustc-1.48.0 on Ubuntu 18.04 and run, the program segfaults with the console output as below.

5 [72, 101, 108, 108, 111], Hello
5 [87, 111, 114, 108, 100], World
Segmentation fault (core dumped)

Suggested Fixes

  1. Make Array trait an unsafe trait
  2. Add warnings about implementing the trait in the docs. If Array trait was not meant to be implemented by users in the first place, clarifying that in the docs would also be helpful.

Thank you for checking out this issue 👍

Edited Jan 01, 2021 by Youngsuk Kim
Assignee Loading
Time tracking Loading