mmap/munmap/mremamp shims #2520

saethlin · 2022-08-30T01:19:03Z

This adds basic support for mmap/mremap/munmap, with the specific goal of testing allocators targeting Linux under Miri.

This supports mmap with MAP_PRIVATE|MAP_ANONYMOUS, and PROT_READ|PROT_WRITE, and explicitly does not support MAP_SHARED (because that's asking for MMIO) as well as any kind of file mapping (because it seems like nobody does MAP_PRIVATE on files even though that would be very sensible). And (officially) we don't support MAP_FIXED, so we always ignore the addr argument.

This supports mremap only when the implementation is allowed to move the mapping (so no MREMAP_FIXED, no MREMAP_DONTUNMAP, and required MREMAP_MAYMOVE), and also when the entirety of a region previously mapped by mmap is being remapped.

This supports munmap but only when the entirety of a region previously mapped by mmap is unmapped.

src/shims/unix/mem.rs

src/machine.rs

bors · 2022-10-19T07:18:45Z

☔ The latest upstream changes (presumably #2322) made this pull request unmergeable. Please resolve the merge conflicts.

RalfJung

Thanks! I left some comments. I think my main concern is that this duplicates book-keeping between the intptrcast module and these new 'mappings', and that seems error-prone. I'd prefer to have only on source of information for mapping between integers and AllocId.

src/intptrcast.rs

src/shims/unix/foreign_items.rs

src/shims/unix/mem.rs

saethlin · 2023-01-15T22:05:41Z

I'm picking this up again. I saw someone looking for this kind of support so I'm probably going to scope this down a bit to avoid some of the annoying questions it raises.

saethlin · 2023-01-27T01:13:51Z

I think the only real thing left to do here is to figure out how to raise an error instead of reporting UB when a user tries to munmap part of a region from mmap. But I have no idea how to do that other than exposing the intptrcast internals. Help?

RalfJung · 2023-01-31T17:55:25Z

src/shims/unix/mem.rs

+            // FIXME: The man page says this returns MAP_FAILED but that is type void*, and this
+            // function returns int.


I assume it's (int)MAP_FAILED then?

🤷 I just re-read the Linux man page for this and it specifies not only that munmap returns -1 on error, but in the docs for mmap:

the value MAP_FAILED (that is, (void *) -1)

RalfJung · 2023-01-31T17:55:49Z

I think the only real thing left to do here is to figure out how to raise an error instead of reporting UB when a user tries to munmap part of a region from mmap. But I have no idea how to do that other than exposing the intptrcast internals. Help?

What exactly is it that needs to be accessed/checked here? I'm seeing a FIXME near a deallocate_ptr call, is that the one you mean? deallocate_ptr is not part of intptrcast though so I am confused?

src/shims/unix/mem.rs

RalfJung · 2023-01-31T18:00:55Z

That sounds like a ton of restrictions that basically make mmap be the same as a miri_allocate. No fundamental objection but I am surprised that that is even useful?

saethlin · 2023-02-01T01:06:11Z

I am surprised that that is even useful?

This is enough to run the test suite for dlmalloc, unmodified. I'm a bit surprised too.

What exactly is it that needs to be accessed/checked here? I'm seeing a FIXME near a deallocate_ptr call, is that the one you mean? deallocate_ptr is not part of intptrcast though so I am confused?

The commits I just pushed addresses this munmap situation. I just wasn't sure how to build the check that the call to munmap is a whole previous allocation, but I guess I can get the information I need about existing allocations based on the access that was added for priroda?

saethlin · 2023-02-11T02:48:31Z

@rustbot ready
I think?

tests/pass-dep/shims/mmap.rs

src/shims/unix/mem.rs

RalfJung · 2023-02-13T21:38:54Z

src/machine.rs

+
+    pub(crate) fn round_up_to_multiple_of_page_size(&self, length: u64) -> Option<u64> {
+        #[allow(clippy::integer_arithmetic)] // page size is nonzero
+        (length.checked_add(self.page_size - 1)? / self.page_size).checked_mul(self.page_size)


The standard library should really have a method for rounding integers up and down...

src/shims/unix/mem.rs

saethlin · 2023-06-13T00:49:43Z

That was weird. I think the windows job timed out or something. It was cancelled, so I restarted it, and it passed this time.

src/shims/unix/mem.rs

RalfJung · 2023-06-16T15:47:13Z

I'm still trying to understand what exactly you want to achieve for munmap.

Clearly this is not a full implementation of mmap. It says in the comments that it only supports the part that behaves like malloc/realloc/free. Why can't we just say that this applies to munmap as well, and that we require the given range to exactly match a previous mmap?

saethlin · 2023-06-16T16:40:13Z

🤦 Yeah I can do that. I previously hadn't figured out how to use the prioroda access to the AllocMap to pull out the information I need to do that.

RalfJung · 2023-06-16T20:08:55Z

You don't even have to do that. Just call dealloc with the right MemoryKind and it should check everything.

saethlin · 2023-06-16T21:02:34Z

But then we will get a false-positive UB report for a munmap call that doesn't perfectly correspond to an mmap call.

RalfJung · 2023-06-17T15:45:02Z

Yeah isn't that fine if we say we only support allocator-style logic?

Or are you saying we should try our hardest to throw "unsupported" instead of UB?

src/shims/unix/mem.rs

saethlin · 2023-06-17T15:49:04Z

Yes, I think we should try our hardest to not say that a program is UB when it's just using something we don't support.

I think it's important that people can trust that when Miri says it has encountered Undefined Behavior that there is a problem in the program.

src/shims/unix/mem.rs

tests/fail/mmap_invalid_dealloc.rs

tests/pass-dep/shims/mmap.rs

RalfJung · 2023-06-20T12:00:48Z

Looking good. :) Please squash the history a bit, then r=me.

saethlin · 2023-06-20T14:18:38Z

@bors r=RalfJung

bors · 2023-06-20T14:18:41Z

📌 Commit 4916a77 has been approved by RalfJung

It is now in the queue for this repository.

bors · 2023-06-20T14:18:48Z

⌛ Testing commit 4916a77 with merge 4b90db3...

bors · 2023-06-20T15:03:10Z

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 4b90db3 to master...

oli-obk reviewed Aug 30, 2022

View reviewed changes

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

saethlin force-pushed the mmap-shim branch from bef8f15 to 70ee96e Compare September 1, 2022 00:26

saethlin added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Sep 4, 2022

saethlin force-pushed the mmap-shim branch 2 times, most recently from 19c0419 to a0a1679 Compare September 22, 2022 04:09

saethlin force-pushed the mmap-shim branch from a9111f5 to 2ceb7b9 Compare October 11, 2022 01:58

oli-obk reviewed Oct 11, 2022

View reviewed changes

src/machine.rs Outdated Show resolved Hide resolved

RalfJung reviewed Oct 29, 2022

View reviewed changes

thomcc mentioned this pull request Nov 2, 2022

Miri's hardcoded PAGE_SIZE is sometimes inaccurate #2644

Closed

saethlin force-pushed the mmap-shim branch from c8fa2f9 to cb19974 Compare January 15, 2023 22:02

saethlin force-pushed the mmap-shim branch 2 times, most recently from b585884 to c6f389f Compare January 16, 2023 04:21

saethlin force-pushed the mmap-shim branch 3 times, most recently from d0d5d53 to 0e5835c Compare January 27, 2023 01:12

saethlin marked this pull request as ready for review January 27, 2023 01:13

RalfJung reviewed Jan 31, 2023

View reviewed changes

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Feb 11, 2023

oli-obk requested changes Feb 13, 2023

View reviewed changes

tests/pass-dep/shims/mmap.rs Outdated Show resolved Hide resolved

src/shims/unix/mem.rs Show resolved Hide resolved

RalfJung reviewed Feb 13, 2023

View reviewed changes

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

saethlin force-pushed the mmap-shim branch 3 times, most recently from 29165b9 to a2f99ce Compare June 12, 2023 20:51

saethlin added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Jun 13, 2023

RalfJung reviewed Jun 16, 2023

View reviewed changes

saethlin removed the S-waiting-on-review Status: Waiting for a review to complete label Jun 16, 2023

saethlin force-pushed the mmap-shim branch from d84f749 to 53df845 Compare June 17, 2023 00:18

RalfJung reviewed Jun 17, 2023

View reviewed changes

src/shims/unix/mem.rs Show resolved Hide resolved

RalfJung reviewed Jun 17, 2023

View reviewed changes

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

RalfJung reviewed Jun 19, 2023

View reviewed changes

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

src/shims/unix/mem.rs Outdated Show resolved Hide resolved

tests/fail/mmap_invalid_dealloc.rs Outdated Show resolved Hide resolved

tests/pass-dep/shims/mmap.rs Show resolved Hide resolved

RalfJung reviewed Jun 19, 2023

View reviewed changes

tests/pass-dep/shims/mmap.rs Show resolved Hide resolved

saethlin added 3 commits June 20, 2023 10:16

mmap/munmap/mremamp shims

17e4c73

Make munmap throw unsup errors instead of trying to work

00fdee2

Improve organization

4916a77

saethlin force-pushed the mmap-shim branch from 14c72bb to 4916a77 Compare June 20, 2023 14:17

bors merged commit 4b90db3 into rust-lang:master Jun 20, 2023

saethlin deleted the mmap-shim branch June 20, 2023 16:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mmap/munmap/mremamp shims #2520

mmap/munmap/mremamp shims #2520

saethlin commented Aug 30, 2022 •

edited

Loading

bors commented Oct 19, 2022

RalfJung left a comment

saethlin commented Jan 15, 2023 •

edited

Loading

saethlin commented Jan 27, 2023

RalfJung Jan 31, 2023

saethlin Feb 1, 2023

RalfJung commented Jan 31, 2023

RalfJung commented Jan 31, 2023

saethlin commented Feb 1, 2023

saethlin commented Feb 11, 2023

RalfJung Feb 13, 2023

saethlin commented Jun 13, 2023

RalfJung commented Jun 16, 2023

saethlin commented Jun 16, 2023

RalfJung commented Jun 16, 2023 via email

saethlin commented Jun 16, 2023

RalfJung commented Jun 17, 2023

saethlin commented Jun 17, 2023

RalfJung commented Jun 20, 2023

saethlin commented Jun 20, 2023

bors commented Jun 20, 2023

bors commented Jun 20, 2023

bors commented Jun 20, 2023

		// FIXME: The man page says this returns MAP_FAILED but that is type void*, and this
		// function returns int.

mmap/munmap/mremamp shims #2520

mmap/munmap/mremamp shims #2520

Conversation

saethlin commented Aug 30, 2022 • edited Loading

bors commented Oct 19, 2022

RalfJung left a comment

Choose a reason for hiding this comment

saethlin commented Jan 15, 2023 • edited Loading

saethlin commented Jan 27, 2023

RalfJung Jan 31, 2023

Choose a reason for hiding this comment

saethlin Feb 1, 2023

Choose a reason for hiding this comment

RalfJung commented Jan 31, 2023

RalfJung commented Jan 31, 2023

saethlin commented Feb 1, 2023

saethlin commented Feb 11, 2023

RalfJung Feb 13, 2023

Choose a reason for hiding this comment

saethlin commented Jun 13, 2023

RalfJung commented Jun 16, 2023

saethlin commented Jun 16, 2023

RalfJung commented Jun 16, 2023 via email

saethlin commented Jun 16, 2023

RalfJung commented Jun 17, 2023

saethlin commented Jun 17, 2023

RalfJung commented Jun 20, 2023

saethlin commented Jun 20, 2023

bors commented Jun 20, 2023

bors commented Jun 20, 2023

bors commented Jun 20, 2023

saethlin commented Aug 30, 2022 •

edited

Loading

saethlin commented Jan 15, 2023 •

edited

Loading