swap_index can write out bounds and return uninitialized memory

[ammaraskar]

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that in swap_index, the length returned by the iterator is used to set the length of the vector:

reorder/src/lib.rs

Lines 46 to 56 in 59ad9be

	let len=bla.len();
	let mut vec=Vec::with_capacity(len);
	let arr:&mut [u32]=unsafe{std::slice::from_raw_parts_mut(vec.as_mut_ptr(),bla.len())};
	for (i,a) in bla.enumerate(){
	arr[a as usize]=i as u32;
	}
	unsafe{
	vec.set_len(len);
	}
	vec

However, as noted in the documentation for ExactSizeIterator's len() function:

This function has the same safety guarantees as the Iterator::size_hint function.

and then size_hint's documentation says:

size_hint() is primarily intended to be used for optimizations such as reserving space for the elements of the iterator, but must not be trusted to e.g., omit bounds checks in unsafe code. An incorrect implementation of size_hint() should not lead to memory safety violations.

Here's an example of some code that will use uninitialized memory through this method:

#![forbid(unsafe_code)]

use reorder::swap_index;

struct IteratorWithWrongLength();

impl Iterator for IteratorWithWrongLength {
    type Item = u32;
    fn next(&mut self) -> Option<Self::Item> { None }
}
impl ExactSizeIterator for IteratorWithWrongLength {
    fn len(&self) -> usize { 1024 }
}

fn main() {
    let v = swap_index(IteratorWithWrongLength());

    println!("{:?}", v);
}

[Shnatsel]

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

[tiby312]

Thank you for the heads up! I submitted a pull request to fix this problem.

[tiby312]

looks like the pull request made it in. thanks for the help