Analyze safety of this use of mmap

#1

As long as the underlying file is not deleted, not truncated, not moved, not resized:

I do not see any obvious UB with @LegionMammal978 's code at Mmap file as &[volatile u8]? - #30 by LegionMammal978

pasted here for convenience

use memmap2::MmapRaw;

pub struct VolatileMmapSlice(MmapRaw);

impl VolatileMmapSlice {
    pub fn new(map: MmapRaw) -> VolatileMmapSlice {
        VolatileMmapSlice(map)
    }

    pub fn into_inner(self) -> MmapRaw {
        self.0
    }

    // SAFETY: index must be less than both isize::MAX and the length of the underlying map
    unsafe fn get_ptr(&self, index: usize) -> *const u8 {
        self.0.as_ptr().add(index)
    }

    // SAFETY: index must be less than both isize::MAX and the length of the underlying map
    unsafe fn get_mut_ptr(&self, index: usize) -> *mut u8 {
        self.0.as_mut_ptr().add(index)
    }

    // SAFETY: the file must not be resized while this function is called
    pub unsafe fn get(&self, n: usize) -> u8 {
        assert!(n < isize::MAX as usize && n < self.0.len());
        self.get_ptr(n).read_volatile()
    }

    // SAFETY: the file must not be resized while this function is called
    pub unsafe fn set(&self, n: usize, v: u8) {
        assert!(n < isize::MAX as usize && n < self.0.len());
        self.get_mut_ptr(n).write_volatile(v)
    }

    // SAFETY: the file must not be resized while this function is called
    pub unsafe fn copy_to(&self, start: usize, len: usize, dst: &mut [u8]) {
        assert!(dst.len() == len);
        let end = start.checked_add(len).unwrap();
        assert!(end < isize::MAX as usize && end < self.0.len());
        for (i, b) in dst.iter_mut().enumerate() {
            *b = self.get_ptr(start + i).read_volatile();
        }
    }

    // SAFETY: the file must not be resized while this function is called
    pub unsafe fn copy_from(&self, start: usize, len: usize, src: &[u8]) {
        assert!(src.len() == len);
        let end = start.checked_add(len).unwrap();
        assert!(end < isize::MAX as usize && end < self.0.len());
        for (i, b) in src.iter().enumerate() {
            self.get_mut_ptr(start + i).write_volatile(*b);
        }
    }
}

Does anyone see any potential UB with the above code ?

#2

It depends whether data races with volatile are still UB, since volatile isn't a synchronization primitive.

And I don't know the answer to that. Everything I've always heard related to it amounts, in my summarization, to "well the compiler doesn't optimize anything when you use volatile so in practice it doesn't break on me", which is not proof of lack of UB.

#3

At this point you could add "not modified" and then even mmapping as &[u8] would be sound.

2 Likes
#4

Looks to me like set and copy_from should take &mut self, since otherwise you enable data races between threads. From the documentation to std::ptr:

it is undefined behavior to perform two concurrent accesses to the same location from different threads unless both accesses only read from memory. Notice that this explicitly includes read_volatile and write_volatile: Volatile accesses cannot be used for inter-thread synchronization.

1 Like
#5

I think this comment is a bit unfair. There are useful situations where we pre allocate a 16GB file, mmap it, and have multiple processes mutate it to pass information.

#6

True. You'd probably need to add a PhantomData<*mut u8> to prevent the automatic Sync impl.

#7

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.