Skip to content

Commit

Permalink
Don't use PROT_EXEC with mmap on macos (#1110)
Browse files Browse the repository at this point in the history
On macos I was running `./miniruby --mmtk -e 'puts "Hello world!"'` and
seeing a panic:

```
thread '<unnamed>' panicked at /src/util/raw_memory_freelist.rs:202:9:
Can't get more space with mmap()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
fatal runtime error: failed to initiate panic, error 5
Abort trap: 6
```

On macos, you can't combine `PROT_EXEC` with `PROT_WRITE` and
`PROT_READ` in `mmap`, it will return a permission denied error. It's
not clear from the `Can't get more space with mmap()` that's what was
happening, but I could see it when inspecting the `ret` from
`mmap_fixed` in `dzmmap_noreplace`.

I found a thread on apple confirming that `PROT_EXEC` doesn't make sense
for macos in this context.
https://forums.developer.apple.com/forums/thread/740017

Note: there are still failing tests for `mmap` on macos. The main change
with this PR is that it no longer panics when runnning a simple hello
world with Ruby on macos. Previously the build wasn't finishing and
would throw the following error:

```
process didn't exit successfully: `/mmtk-core/target/debug/deps/mmtk-ef0f14e36a03fbba` (signal: 11, SIGSEGV: invalid memory reference)
```

Now the tests do finish, with 25 failures.
  • Loading branch information
eileencodes authored Apr 15, 2024
1 parent 86da9e9 commit d291cda
Showing 1 changed file with 8 additions and 2 deletions.
10 changes: 8 additions & 2 deletions src/util/memory.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub fn set(start: Address, val: u8, len: usize) {
/// may corrupt others' data.
#[allow(clippy::let_and_return)] // Zeroing is not neceesary for some OS/s
pub unsafe fn dzmmap(start: Address, size: usize, strategy: MmapStrategy) -> Result<()> {
let prot = PROT_READ | PROT_WRITE | PROT_EXEC;
let prot = MMAP_PROT;
let flags = libc::MAP_ANON | libc::MAP_PRIVATE | libc::MAP_FIXED;
let ret = mmap_fixed(start, size, prot, flags, strategy);
// We do not need to explicitly zero for Linux (memory is guaranteed to be zeroed)
Expand All @@ -58,6 +58,12 @@ const MMAP_FLAGS: libc::c_int = libc::MAP_ANON | libc::MAP_PRIVATE | libc::MAP_F
// MAP_FIXED is used instead of MAP_FIXED_NOREPLACE (which is not available on macOS). We are at the risk of overwriting pre-existing mappings.
const MMAP_FLAGS: libc::c_int = libc::MAP_ANON | libc::MAP_PRIVATE | libc::MAP_FIXED;

#[cfg(target_os = "linux")]
const MMAP_PROT: libc::c_int = PROT_READ | PROT_WRITE | PROT_EXEC;
#[cfg(target_os = "macos")]
// PROT_EXEC cannot be used with PROT_READ on Apple Silicon
const MMAP_PROT: libc::c_int = PROT_READ | PROT_WRITE;

/// Strategy for performing mmap
///
/// This currently supports switching between different huge page allocation
Expand All @@ -77,7 +83,7 @@ pub enum MmapStrategy {
/// This function will not overwrite existing memory mapping, and it will result Err if there is an existing mapping.
#[allow(clippy::let_and_return)] // Zeroing is not neceesary for some OS/s
pub fn dzmmap_noreplace(start: Address, size: usize, strategy: MmapStrategy) -> Result<()> {
let prot = PROT_READ | PROT_WRITE | PROT_EXEC;
let prot = MMAP_PROT;
let flags = MMAP_FLAGS;
let ret = mmap_fixed(start, size, prot, flags, strategy);
// We do not need to explicitly zero for Linux (memory is guaranteed to be zeroed)
Expand Down

0 comments on commit d291cda

Please sign in to comment.