Overflows can unsoundly taint length state in AlignedVec

[HeroicKatora]

Relates to:

rav1d/src/align.rs

Lines 185 to 191 in 7d72409

	pub fn resize(&mut self, new_len: usize, value: T) {
	let old_len = self.len();
	// Resize the underlying vector to have enough chunks for the new length.
	//
	// NOTE: We don't need to `drop` any elements if the `Vec` is truncated since `T: Copy`.
	let new_bytes = mem::size_of::<T>() * new_len;

The following test must fail as it demonstrates memory unsafety, accessing an array way beyond its possible bounds.

#[test]
#[should_panic]
fn align_vec_fails() {
    let mut v = AlignedVec::<u16, Align8<[u8; 8]>>::new();
    v.resize(isize::MAX as usize + 2, 0u16);
    assert_eq!(v.as_slice()[isize::MAX as usize], 0);
}

The problem occurs in release mode, where no overflow checks are enabled. Code must not depend on overflow for correctness. The Safety comment is just incorrect:

        // SAFETY: We've allocated enough space to write
        // up to `new_len` elements into the buffer.

This is untrue. If the calculation for new_bytes overflows, the allocation which actually occurs is much smaller. The fix should be quite small, enforce a limit to the newly requested length such that it never overflows. The case of a zero-sized T can be ignored, this will never allocate at all.

I'm intending to create a PR soonish.

[HeroicKatora]

Expanding on the memory safety issue by stepping through the test: After resizing v the inner vector has a length of 1. This is because (isize::MAX as usize + 2) * 2 wraps to the result 2usize. Consequently, the new byte length is calculated as 1. The following assert succeeds at this point:

    assert_eq!(v.inner.len(), 1);

The allocation consequently has a size of 8 bytes. The method nevertheless continues and the initialization loop offsets the base pointer of the allocation way past those 8 bytes and writes the requested number into each memory. This is already UB. Note that the later access in as_slice()[isize::MAX as usize] will also wrap in memory but the safety requirement of slices has been violated in the call to from_raw_parts so this is not necessarily detected. In any case, sometimes this test produces:

Caused by:
  process didn't exit successfully: `/tmp/rav1d/target/release/deps/rav1d-ddebe794449c1390` (signal: 11, SIGSEGV: invalid memory reference)

[Lokathor]

One possible "quick fix" is the classic size_of::<T>().checked_mul(new_len).unwrap()