Actually mark mmap and related functions as unsafe

[kevinmehall]

The nix::sys::mman::mmap documentation says

Calls to mmap are inherently unsafe, so they must be made in an unsafe
block.

however, the function was not actually marked unsafe.

• munmap should also be unsafe for obvious reasons.
• madvise should be unsafe because of the MADV_DONTNEED flag.
• mlock was already marked unsafe
• munlock and msync don't strictly need to be unsafe despite
  taking pointers AFAICT, but are marked unsafe for consistency and in
  case they add additional flags in the future.

[breaking-change]

[kamalmarhubi]

Thanks for the PR!

Re mmap and munmap, definitely agree they should be marked unsafe. Also agree madvise should be, as I didn't realise that madvise(MADV_DONTNEED) had this scary behaviour (emphasis mine):

After a successful MADV_DONTNEED operation, the semantics of memory access in the specified region are changed: subsequent accesses of pages in the range will succeed, but will result in either repopulating the memory contents from the up-to-date contents of the underlying mapped file (for shared file mappings, shared anonymous mappings, and shmem-based techniques such as System V shared memory segments) or zero-fill-on-demand pages for anonymous private mappings.

Could you explain why mlock needs to be unsafe? And what kind of things do you imagine could be added to munlock and msync to require marking them unsafe?

[kamalmarhubi]

Oh and could you add a note to the changelog mentioning these changes? Thanks!

[kevinmehall]

mlock was already marked unsafe before this PR (the only unsafe function in the module). I think mlock and munlock should be consistent -- these functions take raw pointers but can't break Rust memory safety with them as far as I can tell. I went with unsafe here, but could go either way.

msync takes a flags argument, and I can't tell what MS_INVALIDATE does well enough to determine whether it's safe. The flags could be expanded in the future to add additional operations that may be designed without Rust semantics in mind.

[kamalmarhubi]

mlock was already marked unsafe before this PR (the only unsafe function in the module). I think mlock and munlock should be consistent -- these functions take raw pointers but can't break Rust memory safety with them as far as I can tell. I went with unsafe here, but could go either way.

I'd lean towards marking them safe, but maybe unsafe is safer initially. I'm not really all that familiar with semantics of this stuff.

msync takes a flags argument, and I can't tell what MS_INVALIDATE does well enough to determine whether it's safe.

Oh interesting. It sounds as though it could cause unsafety in another program that mapped an overlapping region of the same file. Marking it unsafe for now sounds right. We should ask someone with a better handle on what's considered unsafe / undefined eventually.

[kamalmarhubi]

Summary: I'd be fine with marking these all as unsafe, since removing unsafe later is not a breaking change.

[kevinmehall]

Ok, added a changelog entry.

[kamalmarhubi]

Thanks @kevinmehall!

@homu r+

[homu]

📌 Commit 902f281 has been approved by kamalmarhubi

[homu]

⌛ Testing commit 902f281 with merge 0eef651...

[homu]

☀️ Test successful - status