Skip to content

Handle errors from mmap and mprotect due to Apple Hardened Runtime.#93

Merged
thejayps merged 7 commits intomasterfrom
branch/2022-12-23/hardened-runtime
Jan 13, 2023
Merged

Handle errors from mmap and mprotect due to Apple Hardened Runtime.#93
thejayps merged 7 commits intomasterfrom
branch/2022-12-23/hardened-runtime

Conversation

@gareth-rees
Copy link
Member

On Apple Silicon, when Hardened Runtime is in force and the application lacks the "Allow Unsigned Executable Memory Entitlement", mmap and mprotect return EACCES when an attempt is made to create or modify memory so that it is simultaneously writable and executable.

This commit handles these cases by retrying the operation without the PROT_EXEC flag, and setting global variables so that future operations omit the flag.

Work towards #75 (Issue with Apple silicon write-xor-execute memory requirements)

Note that this doesn't completely address #75 because there is no provision for MAP_JIT. However, in the absence of a clear customer requirement it's difficult for me to pick a good solution. This commit is a self-contained change that allows developers to work with the MPS on Apple Silicon without having to configure the Allow Unsigned Executable Memory Entitlement.

This pull request supersedes pull request #82.

On Apple Silicon, when Hardened Runtime is in force and the
application lacks the "Allow Unsigned Executable Memory Entitlement",
mmap and mprotect return EACCES when an attempt is made to create or
modify memory so that it is simultaneously writable and executable.

This commit handles these cases by retrying the operation without the
PROT_EXEC flag, and setting global variables so that future operations
omit the flag.
@gareth-rees
Copy link
Member Author

I think this ought to be good to merge now — the problem on #82 was "we are currently unable to approve because we can't build on all target platforms" but I can confirm that tests pass on xca6ll, Travis shows tests passing on lia6gc, lia6ll, lii6gc, lii6ll, and xci6ll, and Travis shows the product building on w3i6mv. The risk is very low that this PR breaks tests on Windows because it only touches protix.c and vmix.c, neither of which is used in the Windows build.

rptb1 added 2 commits January 10, 2023 17:07
… the top-level Makefile, breaking FreeBSD builds. Adding warning to prevent similar mistakes.
@rptb1
Copy link
Member

rptb1 commented Jan 11, 2023

We should compare this work with #77

rptb1 added 2 commits January 11, 2023 08:20
…3/hardened-runtime to get Travis to test this branch on Windows before merging.
…3/hardened-runtime to get Travis to test this branch on FreeBSD before merging.
@rptb1
Copy link
Member

rptb1 commented Jan 11, 2023

The risk is very low that this PR breaks tests on Windows

I've merged branch/2023-01-05/travis-windows and branch/2023-01-10/travis-freebsd so we'll find out.

@rptb1
Copy link
Member

rptb1 commented Jan 11, 2023

I've merged branch/2023-01-05/travis-windows and branch/2023-01-10/travis-freebsd so we'll find out.

Well, we found out something else! https://app.travis-ci.com/github/Ravenbrook/mps/jobs/593019182#L264-L267

protix.c:97:13: error: implicit conversion loses integer precision:
      'sig_atomic_t' (aka 'long') to 'int' [-Werror,-Wshorten-64-to-32]
    flags = prot_all;
          ~ ^~~~~~~~

@thejayps
Copy link
Contributor

thejayps commented Jan 12, 2023

Executing Procedure

  • Is there a permanent visible document (issue, job), referenced by the branch, recording the problem that is solved by the changes in the branch? yes
  • Does the branch solve the problem? partly, @gareth-rees himself states that the issue isn't fully addressed.

Is there an automated test case that demonstrates the problem (without the branch) and that that the problem is solved (with the branch)? Not obviously available. A test running on apple silicon should be made available and referenced from the pull request

If there are interface changes, are they documented? yes

If the changes are significant and user-visible, is there an update to the release notes (manual/source/release.rst)? yes

Has there been a code review? yes

Has the contributor licensed their work? unclear - note that changes to the "Contributing to the MPS" made alongside the development of this procedure did not apply at the time of this pull request.

By default, the work is licensed if the contributor has not expressed any kind of variation on the licensing of their work from the material to which they are contributing. (See "Licensing" in "Contributing to the MPS".)

If they have, talk to them or get someone to talk to them about the matter. Do not proceed.. .... proceeding from this point with @rptb1 's permission

Does the branch build and pass tests on all target platforms?

If the branch is in the Ravenbrook MPS repo on GitHub then Travis CI should have run builds. Look for a successful build in the Travis CI build history for the repo. If there is a failed build you should not execute this procedure, but talk to the contributor about fixing the branch.

If the branch is in the Ravenbrook MPS repo on GitHub and Travis builds are missing, inform sysadmins that Travis CI isn't functioning.

If you have no build and test results, you can still execute this procedure, with caution.

... yes

Does the branch merge cleanly in to master and pass tests on all target platforms?

Travis CI should have run builds of the pull request (i.e. of a merge with master). To check, either:

Look for "All checks have passed" in the pull request on GitHub. Expand "Show all checks", and look for build success messages from Travis CI.
Look for a successful build in the Travis CI build history for the repo.
Success by Travis CI is a strong indication that this procecure will be quick and successful.

If Travis builds failed, you can still execute this procedure if you believe that the failure is due to merge conflicts that you are willing to resolve.

If Travis builds are missing, inform sysadmins that Travis CI isn't functioning.

If you have no build and test results for the merge, then you can still execute this procedure if:

you believe there are only merge conflicts,
you're willing to try to resolve those conflicts, and
you're prepared to test on all target platforms.

... travis build failed : https://app.travis-ci.com/github/Ravenbrook/mps/jobs/593083274#L901-L904 but @rptb1 gives permission to proceed with the procedure, as this is a rare longstanding failure that we are aware of: #108 .

@thejayps
Copy link
Contributor

@gareth-rees Please confirm that you agree to this: https://github.com/Ravenbrook/mps/blob/master/contributing.rst#licensing

Copy link
Member

@rptb1 rptb1 left a comment

Choose a reason for hiding this comment

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

I hereby approve the merge of this branch in spite of the intermittent build failure.

@thejayps thejayps merged commit 43a856c into master Jan 13, 2023
@thejayps
Copy link
Contributor

Attempt 2 at pull request procedure

Noting only questions where the answer isn't "yes"

2: partially
3: no
7: no - request to @gareth-rees pending
9: no

@rptb1
Copy link
Member

rptb1 commented Jan 13, 2023

9: no

But I said to go ahead because the build failures are very likely to be a result of #59

@rptb1
Copy link
Member

rptb1 commented Jan 13, 2023

3: no

We're discussing (keybase://chat/ravenbrook#mps/2836) how we might arrange build automation and CI for Apple Silicon.

Copy link
Contributor

@thejayps thejayps left a comment

Choose a reason for hiding this comment

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

Reviewed travis.yml and Makefile.in post-merge after realising they had been accidentally included. See #97 (comment)

Looks good.

@rptb1 rptb1 added the arch.a6 Relates to arm64/aarch64 label Feb 13, 2023
jollaitbot pushed a commit to sailfishos-mirror/emacs that referenced this pull request Sep 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

arch.a6 Relates to arm64/aarch64

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants