mmap-alloc: Improve tests for permissions, fix realloc bugs #87

joshlf · 2017-09-21T22:53:34Z

Add the ability to parse /proc/<pid>/maps on Linux, and incorporate this into existing tests in order to verify permissions
Remove checks for allocation at the 0 page, as it turns out that these are unnecessary on all platforms that we support (Linux, Mac, Windows)
Change the way boolean configuration functions work for MapAllocBuilder (instead of xxx() and no_xxx(), we now supply a single xxx(xxx: bool) method that accepts a boolean indicating whether the configuration should be on or off)
Move tests module into separate file

Closes #83.
Closes #72 and #80 by making them unnecessary since this commit removes the use cases for mark_unused and alloc_helper.

For posterity: A previous version of the code to verify memory permissions attempted to access a region in order to generate a SIGSEGV signal, and catch that signal. I couldn't get that code working, so I switched to parsing /proc/<pid>/maps instead. If somebody wants to take a look and maybe try to get it working, the draft I had before I switched is here.

joshlf · 2017-09-21T22:57:05Z

@Dr-Emann Just wanted to give you a heads up about this - I came across a line about Linux guaranteeing non-null addresses from mmap while reading the manpage, and when I dug further, I realized it's guaranteed on POSIX in general (meaning Mac as well). This unfortunately means that a lot of the work you did in the previous PR is taken out in this PR as it's unnecessary (the same goes for some code that I wrote previously to handle the same case for allocation). I'm sorry to have you go through all of that just to have the code removed a couple of days later.

I will say that I'm not 100% convinced about the manpage being correct - both @ezrosent and I have recollections (mine in the form of a comment in test.rs, actually) that Linux has, on occasion, returned null from mmap. However, when I tested this on play.rust-lang.org, I couldn't reproduce it. I'm going to investigate more fully before I merge this PR.

UPDATE: it's definitely guaranteed on Mac. I'm still trying to determine whether mremap is guaranteed to not return null.

Dr-Emann · 2017-09-22T04:03:34Z

Can I cast my vote against formatting multiline functions definitions this way? Besides my personal bias (I think it's incredibly ugly), it's no longer the recommended format for rust code, and using the newer rustfmt-nightly formats it much better (see also, the format button on the playground)

Dr-Emann · 2017-09-22T04:16:36Z

As for mapping returning null: don't worry about it. It was fun figuring how to deal with it, even if it would never happen.

fyi, I did mention that posix seemed to disallow returning null from mmap without MAP_FIXED in the description of my pull request. 😜

joshlf · 2017-09-22T04:42:11Z

Can I cast my vote against formatting multiline functions definitions this way? Besides my personal bias (I think it's incredibly ugly), it's no longer the recommended format for rust code, and using the newer rustfmt-nightly formats it much better (see also, the format button on the playground)

Oh, absolutely! I've been using stable rustfmt, but I'm happy to switch to rustfmt-nightly; I just didn't remember it was a thing so I never installed it.

fyi, I did mention that posix seemed to disallow returning null from mmap without MAP_FIXED in the description of my pull request. 😜

Shit you're right - I see that now (and I think I know why I didn't catch it before). That's totally my bad. Your point in that original comment about explicitly mapping a null page is a good one! I think I'll add that point to my comment in this PR so if it becomes an issue in the future, we'll have a comment with an idea of how to fix it.

EDIT: Actually, mapping the null page at the beginning wouldn't work - if it failed, that might be because somebody else had already mapped it. If that person later unmapped it, we'd still be at a risk of getting null pages back from mmap or mremap.

joshlf · 2017-09-22T04:49:30Z

OK, looks like we're good on Mac with respect to mapping the null page.

Dr-Emann · 2017-09-22T06:42:36Z

mmap-alloc/src/lib.rs

+ // we're configured to allocate un-readable or un-writable memory - we'll need to be
+ // able to read/write in order to copy the data.
+ protect(ptr, old_size, perms::PROT_READ);
+ protect(new_ptr, new_size, perms::PROT_WRITE);


Even though reallocation should be infrequent, I'm not sure how I feel about 3 guaranteed syscalls, when in most cases none would be needed (I'd expect most allocators to expect read+write)

Good call. I'll check the perms first.

Dr-Emann · 2017-09-22T06:44:37Z

mmap-alloc/src/tests.rs

+ let output = Command::new("cat")
+ .arg(format!("/proc/{}/maps", unsafe { libc::getpid() }))
+ .output()
+ .unwrap();


Is there a reason to use cat vs opening the file ourselves with std::fs::File?

This was adapted from an earlier version of the function whose job was to get the permissions for a particular address block, and thus was a more complicated command involving a full sequence of commands chained together with pipes. I honestly didn't even notice what this ended up getting whittled down to - I'll change it.

joshlf · 2017-09-26T04:05:01Z

I've been so far unable to find documentation conclusively stating that mremap won't return null. However, I did find that in the Linux 4.13.3 source, mremap, when called without REMAP_FIXED, works by calling get_unmapped_area. I can't see conclusively from the source that get_unmapped_area never returns null. However, what I can see is that get_unmapped_area is also used to implement mmap, and we're already confident that mmap (without the MAP_FIXED flag) does not return null. That, combined with the fact that if it turns out we're wrong, we'll catch it in an unconditional assert! in the Linux implementation of the private map function, makes me confident enough to merge this.

joshlf · 2017-09-26T04:05:24Z

@Dr-Emann, I'll let you weigh in first, but otherwise I'm ready to merge this.

Dr-Emann · 2017-09-26T04:18:28Z

mmap-alloc/src/lib.rs

+ return Ok(ptr);
+ }
+ }
+ }


Since we're not overriding shrink_in_place for macos either, I don't think this makes sense to have. The cases which would be handled by the default implementation (and are handled for macos and windows) should already be handled by the old_size == new_size check above, which checks if the old layout and the new layout both round up to the same size.

Sorry, I hoped there would be more context in the preview. The block starts here

I fixed this by implementing shrink_in_place on Mac and simplifying the logic here. How does that look?

Dr-Emann · 2017-09-26T04:27:06Z

mmap-alloc/src/lib.rs

@@ -255,6 +231,9 @@ impl Default for MapAllocBuilder {
 pub struct MapAlloc {
 pagesize: usize,
 huge_pagesize: Option<usize>,
+ #[cfg_attr(target_os = "linux", allow(unused))] read: bool,
+ #[cfg_attr(target_os = "linux", allow(unused))] write: bool,
+ #[cfg_attr(target_os = "linux", allow(unused))] exec: bool,


It seems unfortunate to have this duplication (the same info should be stored in the perms field. Could we either:

Use helper fns like perms::can_read(Perm)

Make Perm a real type (maybe using bitflags, with a method to_os_perm() which returns the right type depending on the OS? The custom type could support tests like perm.contains(perms::READ).

I agree that this is a tad ugly, but given that we almost exclusively use perms as opaque, and only use the boolean functionality in one place (realloc), I think it's better to keep it simple.

Dr-Emann · 2017-09-26T04:28:58Z

mmap-alloc/src/lib.rs

+ debug_assert!(
+ layout.size() > 0 && new_layout.size() > 0,
+ "grow_in_place: size of layout and new_layout must be non-zero"
+ );


I think I'd split this into two asserts, but I don't have a strong opinion either way.

Dr-Emann · 2017-09-26T04:29:35Z

mmap-alloc/src/lib.rs

+ debug_assert!(
+ layout.size() > 0 && new_layout.size() > 0,
+ "shrink_in_place: size of layout and new_layout must be non-zero"
+ );


Likewise for splitting the assert.

Dr-Emann · 2017-09-26T04:38:52Z

mmap-alloc/src/lib.rs

+ new_size,
+ perms::get_perm(self.read, true, self.exec),
+ );
+ }


I'd prefer to split this, so if reads are allowed, but not writes, there's no reason to fix permissions on the old allocations, only the new one, to allow writing, then reset them after. Likewise, for the opposite, if writes are allowed, but not reads, we only have to fix permissions on the old allocation, and never have to reset them back.

Again, missing important context in the short preview. Starts here

- Add the ability to verify memory permissions by parsing /proc/<pid>/maps on Linux and using the VirtualQuery function on Windows - Remove checks for allocation at the 0 page, as it turns out that these are unnecessary on all platforms that we support (Linux, Mac, Windows) - Change the way boolean configuration functions work for MapAllocBuilder (instead of xxx() and no_xxx(), we now supply a single xxx(xxx: bool) method that accepts a boolean indicating whether the configuration should be on or off) - Fix bugs in realloc tests on Windows - Move tests module into separate file - Remove test-no-std feature Closes #83 Closes #72 and #80 by making them unnecessary since this commit removes the use cases for mark_unused and alloc_helper

Dr-Emann · 2017-09-29T23:37:50Z

I still think the read/write/exec bools are ugly, but other than that, it all looks good, I'd say this is ready to merge in my eyes.

joshlf force-pushed the mmap-alloc-cleanup branch from 09e9ca2 to bcd420c Compare September 22, 2017 03:06

joshlf force-pushed the mmap-alloc-cleanup branch from bcd420c to b9c362c Compare September 22, 2017 05:09

Dr-Emann reviewed Sep 22, 2017

View reviewed changes

joshlf force-pushed the mmap-alloc-cleanup branch 9 times, most recently from ac3c722 to 3e5636d Compare September 25, 2017 21:32

Dr-Emann suggested changes Sep 26, 2017

View reviewed changes

joshlf force-pushed the mmap-alloc-cleanup branch 2 times, most recently from fb060a2 to 900dae1 Compare September 26, 2017 21:24

joshlf force-pushed the mmap-alloc-cleanup branch from 900dae1 to 20e6516 Compare September 28, 2017 19:43

joshlf added the component-mmap-alloc label Sep 29, 2017

joshlf merged commit 948fadd into master Sep 29, 2017

joshlf deleted the mmap-alloc-cleanup branch September 29, 2017 23:40

joshlf mentioned this pull request Sep 29, 2017

mmap-alloc: Document that alloc_helper requires a page-multiple size #80

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mmap-alloc: Improve tests for permissions, fix realloc bugs #87

mmap-alloc: Improve tests for permissions, fix realloc bugs #87

joshlf commented Sep 21, 2017 •

edited

Loading

joshlf commented Sep 21, 2017 •

edited

Loading

Dr-Emann commented Sep 22, 2017

Dr-Emann commented Sep 22, 2017

joshlf commented Sep 22, 2017 •

edited

Loading

joshlf commented Sep 22, 2017

Dr-Emann Sep 22, 2017

joshlf Sep 22, 2017

Dr-Emann Sep 22, 2017

joshlf Sep 22, 2017

joshlf commented Sep 26, 2017

joshlf commented Sep 26, 2017

Dr-Emann Sep 26, 2017

Dr-Emann Sep 26, 2017

joshlf Sep 26, 2017

Dr-Emann Sep 26, 2017

joshlf Sep 26, 2017

Dr-Emann Sep 26, 2017

joshlf Sep 29, 2017

Dr-Emann Sep 26, 2017

joshlf Sep 29, 2017

Dr-Emann Sep 26, 2017

Dr-Emann Sep 26, 2017

joshlf Sep 26, 2017

Dr-Emann commented Sep 29, 2017

mmap-alloc: Improve tests for permissions, fix realloc bugs #87

mmap-alloc: Improve tests for permissions, fix realloc bugs #87

Conversation

joshlf commented Sep 21, 2017 • edited Loading

joshlf commented Sep 21, 2017 • edited Loading

Dr-Emann commented Sep 22, 2017

Dr-Emann commented Sep 22, 2017

joshlf commented Sep 22, 2017 • edited Loading

joshlf commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joshlf commented Sep 26, 2017

joshlf commented Sep 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dr-Emann commented Sep 29, 2017

joshlf commented Sep 21, 2017 •

edited

Loading

joshlf commented Sep 21, 2017 •

edited

Loading

joshlf commented Sep 22, 2017 •

edited

Loading