Better terminology re: safety about memmap2-rs

[adamreichold]

Related to this, we should probably also discuss safety of the various constructor methods for Mmap and MmapMut in the context of how memory maps interact with Rust's aliasing guarantees, c.f. https://users.rust-lang.org/t/how-unsafe-is-mmap/19635.

TLDR, if I create a unique or shared slice reference, I currently need to ensure via outside means that no other process modifies the underlying file with those changes propagating into my mapping, so that an &[u8] becomes effectively mutably aliased.

While doing so, we could also stop allowing the missing_safety_doc Clippy lint.

[RazrFalcon]

Yes, I'm also not sure what should happen when we open the same file twice. On the other hand, all memmap methods are unsafe for a reason.

[scottlamb]

You can't use the raw pointers without saying unsafe. Thus, constructing a MmapRaw is sound where constructing a Mmap is not unless the caller can provide the necessary guarantees. It would be possible to make MmapRaw accessors which provide a safe copying interface (without requiring those guarantees, using unsafe soundly internally). I'd like a sound, safe mmap interface.

[RazrFalcon]

Hmm... I'm not the one who wrote this docs, so I don't have any comments here either.
I would have to test it myself and if this is true, I guess we can update the docs.

[scottlamb]

I agree SIGBUS is not a memory safety issue. It is a non-safety-related caveat that should be documented. Performance implications of major page faults probably fall in that category also.

Yes, I'm also not sure what should happen when we open the same file twice.

I think it's necessary and sufficient that caller guarantees when creating a Mmap or MmapMut that the file isn't changing through any other means—other processes, direct use of this file descriptor (or another pointing to the same inode), or other MmapMut instances. Then the safe Deref/DeRefMut implementations are sound.

On the other hand, all memmap methods are unsafe for a reason.

There are a couple safe ones (correctly, I think):

• memmap2::map_anon is fine because the memory can't change through external means
• memmap2::map_raw is fine because MmapRaw doesn't construct references.

If there were helpers on MmapRaw which copied data into/out of the mapping by std::ptr::copy_nonoverlapping on raw pointers, those could also be safe. They'd never construct a reference to something that can be mutated behind Rust's back. (I think it'd be good to add these and separate MmapRaw into MmapRaw + MmapRawMut.)

[RazrFalcon]

Honestly, I don't understand why we have MmapRaw in the first place. All we have to do is to add as_ptr/as_mut_ptr to Mmap/MmapMut.

@diwic Hi. Could you clarify why you choose to create MmapRaw instead of adding as_ptr/as_mut_ptr to Mmap/MmapMut?

[scottlamb]

I don't know the history, but I like the idea of being able to use mmap in a purely-safe way, so I like the idea of having ones that don't expose references (instead only having copying functions).

[RazrFalcon]

so I like the idea of having ones that don't expose references

Well, they expose raw pointers, which is also not safe.

[RazrFalcon]

Do I understand correctly, that you're suggesting to mark map_raw as safe, because it technically safe (raw pointer dereference is up to the user). And map_anon is safe, because no other process can access it?

[scottlamb]

Those already are marked as safe: map_raw, map_anon.

[RazrFalcon]

Ok... I guess I'm starting to understand the issue =)

Would you be interested in making a PR? I bet I will mess it up if I do it myself.

[scottlamb]

Sure!

[adamreichold]

It would be possible to make MmapRaw accessors which provide a safe copying interface (without requiring those guarantees, using unsafe soundly internally). I'd like a sound, safe mmap interface.

What would you see as the primary benefit of such an interface compared to say using fs::File with read_at and write_at? Personally, I can see reduced system call overhead. Other than that, the point of mmap appears to be the ability to access file content as if it was process memory and in Rust this seems to imply as a byte slice.

[scottlamb]

Yeah, just speed, primarily through reduced system call overhead. I saw an article suggesting userland memcpy is faster than kernel memcpy because of SIMD, but I think that's pretty minor in comparison.

[diwic]

Honestly, I don't understand why we have MmapRaw in the first place. All we have to do is to add as_ptr/as_mut_ptr to Mmap/MmapMut.

@diwic Hi. Could you clarify why you choose to create MmapRaw instead of adding as_ptr/as_mut_ptr to Mmap/MmapMut?

So I use MmapRaw for https://github.com/diwic/shmem-ipc - between untrusted processes. So that means, that the other process could at any time change the data, so creating references at any point is potentially unsound. With Mmap / MmapMut it's extremely easy to create such references, it's just a deref away, the compiler could do it without you thinking about it - e g, the len() function is only available by first creating a &[u8].

That was the reasoning. If you must avoid creating references to the shared data, then Mmap / MmapMut does not have a good API because it makes it too easy to do just that.

[diwic]

The docs for MmapRaw::as_ptr() and MmapRaw::as_mut_ptr() both say:

Safety

To safely dereference this pointer, you need to make sure that the file has not been truncated since the memory map was created.

However, this isn't true in the Rust sense of the word safety: if the file is truncated, the memory mapping itself is not changed. So accessing that memory will simply cause the process to be killed with SIGBUS(1) or equivalent. That's not actually a memory safety issue as no memory will actually be read or written.

To avoid confusion, I think it'd be good to keep this warning, but rename the section to something else and describe why truncation isn't directly a memory safety issue.

Even if that statement is true in Linux, I'm not sure it is correct for all supported platforms. E g, on Windows, it is recommended to use exceptions, and to which degree exceptions are safe or unsafe in Rust is beyond me...

[RazrFalcon]

@diwic Thanks for the explanation. I agree with this decision.