map_anon() can unsoundly create overlarge slices in safe code

[LegionMammal978]

On i686-unknown-linux-gnu, mmap() with MAP_ANONYMOUS is able to serve requests of 0x80000000 or more bytes. MmapMut::map_anon() and <MmapMut as Deref>::deref() can create a slice of this length in safe code:

/*
[dependencies]
memmap2 = "=0.5.4"

cargo run --target i686-unknown-linux-gnu
*/

use memmap2::MmapMut;

fn main() {
    let map: MmapMut = MmapMut::map_anon(0x80000000).unwrap();
    let slice: &[u8] = &*map;
    println!("{:#x}", slice.len()); // 0x80000000
}

However, <MmapMut as Deref>::deref() uses std::slice::from_raw_parts(), which has this safety precondition:

Behavior is undefined if any of the following conditions are violated: [...]

• The total size len * mem::size_of::<T>() of the slice must be no larger than isize::MAX. See the safety documentation of pointer::offset.

Since isize::MAX == 0x7fffffff on i686-unknown-linux-gnu and other 32-bit targets, it is undefined behavior to create this 0x80000000-byte slice using std::slice::from_raw_parts(). Compiler optimizations can behave erratically if this precondition is violated.

The most straightforward solution would be to add assertions to MmapOptions::map(), MmapOptions::map_exec(), MmapOptions::map_mut(), MmapOptions::map_copy(), and MmapOptions::map_copy_read_only(), and MmapOptions::map_anon() that the provided or computed length is no greater than isize::MAX. MmapOptions::map_raw() does not need this assertion, since MmapRaw does not provide slices in safe code. (It may also be worthwhile to add a method that produces an anonymous MmapRaw not checked against isize::MAX.)

[RazrFalcon]

I guess you're referring to:

However, some 32-bit and 16-bit platforms may successfully serve a request for more than isize::MAX bytes with things like Physical Address Extension. As such, memory acquired directly from allocators or memory mapped files may be too large to handle with this function.

An assertions would be too sporadic. I guess we should simply return an error and update the docs.

[RazrFalcon]

I cannot find how std::fs::read handles this. It should have the exact issue.

[RazrFalcon]

@adamreichold Hi! Can you comment on this one? It fills like half of the bugs we had are 32-bit related. But disabling its support completely would be too much.

[LegionMammal978]

I cannot find how std::fs::read handles this. It should have the exact issue.

Following the call chain, std::fs::read()

1. calls <File as Read>::read_to_end(),
2. which calls Vec::<u8>::reserve(),
3. which calls RawVec::<u8>::reserve(),
4. which calls RawVec::reserve::do_reserve_and_handle::<u8, Global>(),
5. which calls RawVec::<u8>::grow_amortized(),
6. which calls alloc::raw_vec::finish_grow::<Global>(),
7. which calls alloc::raw_vec::alloc_guard(),
8. which checks the size in bytes against isize::MAX as usize.

Even if it fails to get the file size, it

1. calls std::io::default_read_to_end(),
2. which repeatedly calls Vec::<u8>::extend_from_slice(),
3. which calls <Vec<u8> as SpecExtend<&u8, slice::Iter<'_, u8>>::spec_extend(),
4. which calls Vec::<u8>::append_elements(),
5. which calls Vec::<u8>::reserve(),
6. which checks the size as before.

[RazrFalcon]

Nice find! I guess I will borrow this function.

[adamreichold]

@adamreichold Hi! Can you comment on this one? It fills like half of the bugs we had are 32-bit related. But disabling its support completely would be too much.

I think it is a straight-forward error on our side. We do have the necessary check for file-based mappings in MmapOptions::get_len, but not in MmapOptions::len.

[adamreichold]

I think our check in get_len was actually too lenient as well. I still think this is a straight-forward bug though, as exposing mmap (2), we are in the memory allocator business need to take the same care as other implementors of such functionality. 😓