Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't use PROT_EXEC with mmap on macos #1110

Merged
merged 1 commit into from
Apr 15, 2024

Conversation

eileencodes
Copy link
Contributor

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.

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.
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I think for now getting something to work is better than nothing. But we do need to consider how MMTk handles code allocation for JIT.

MMTk might need to do this on macOS https://developer.apple.com/documentation/apple-silicon/porting-just-in-time-compilers-to-apple-silicon#Enable-the-JIT-entitlements-for-the-Hardened-Runtime

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the PR! I wonder if we should do things differently for Intel Macs vs Apple Silicon? Though maybe it makes more sense to have the same mechanism regardless of the underlying architecture. @qinsoon Do you have any thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we should do things differently for Intel Macs vs Apple Silicon?

Maybe. But our support for intel macOS is minimal as well so this is not a big concern for now.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@qinsoon qinsoon added this pull request to the merge queue Apr 15, 2024
Merged via the queue into mmtk:master with commit d291cda Apr 15, 2024
23 checks passed
@eileencodes eileencodes deleted the remove-PROT_EXEC-on-macos branch April 15, 2024 13:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants