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

[CP] Make sure FFI works on standalone Mac OS X #54035

Closed
mraleph opened this issue Nov 13, 2023 · 12 comments
Closed

[CP] Make sure FFI works on standalone Mac OS X #54035

mraleph opened this issue Nov 13, 2023 · 12 comments
Assignees
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable

Comments

@mraleph
Copy link
Member

mraleph commented Nov 13, 2023

Commit(s) to merge

6c92ce6

Target

stable

Prepared changelist for beta/stable

https://dart-review.googlesource.com/c/sdk/+/335881

Issue Description

Trying to use FFI callbacks on Mac OS X using standalone dart binary causes a crash due to code signing violation. This is a regression caused by bd57548 which changed how callback pages are allocated, but did not accomodate for Mac OS X specific requirements (i.e. that executable memory should be allocated with MAP_JIT flag when hardened runtime is active).

The issue slipped through testing because we did not have coverage for hardened runtime enforced behaviors on CI - but we have now addressed this issue (see 4ce2362 and 906ae3a)

What is the fix

Allocate executable memory for callback pages using MAP_JIT flag on Mac OS X.

Why cherry-pick

Users can't use FFI callbacks on Mac OS X in standalone CLI Dart.

Risk

low

Issue link(s)

#53928

Extra Info

No response

@mraleph mraleph added the cherry-pick-review Issue that need cherry pick triage to approve label Nov 13, 2023
@mraleph
Copy link
Member Author

mraleph commented Nov 13, 2023

@athomas it is not clear to me if I should edit CHANGELOG.md or not given that 3.2 is not yet released.

@sortie
Copy link
Contributor

sortie commented Nov 13, 2023

Do you want this to be in 3.2.0? Or 3.2.1?

if you want it in 3.2.0, you don't need the changelog. Add the Changelog-Exempt footer with an explanation and loop in @itsjustkevin who needs to reauthor the 3.2.0 release after you submit it (ideally ~today/~tomorrow since I think we're set to ship 3.2.0 on wednesday)

if you want it in 3.2.1, wait until 3.2.0 is out and then just follow the normal procedure of adding a changelog entry.

@mraleph
Copy link
Member Author

mraleph commented Nov 13, 2023

It would be nice to rebuild 3.2.0 if possible and not ship it with a known crasher.

@itsjustkevin
Copy link
Contributor

@mraleph is this a must have or nice to have? We are a bit late in the cycle and I am worried about introducing risk. However if we are okay with it, we can make it happen. I defer to @mit-mit

@mraleph
Copy link
Member Author

mraleph commented Nov 14, 2023

@itsjustkevin Given that FFI is effectively broken on 3.2 standalone I would classify it this as must have.

I have also made sure that we actually have test coverage for the fix. We managed to regress with bd57548 because we thought we have coverage, but we did not have it. This is now fixed.

@itsjustkevin
Copy link
Contributor

@mraleph please get a review on the CL so we can get the process started.

@mraleph
Copy link
Member Author

mraleph commented Nov 14, 2023

@itsjustkevin done.

@mit-mit
Copy link
Member

mit-mit commented Nov 14, 2023

@mraleph is this only macOS, or also iOS? If only macOS, I think we can consider doing a quick patch release (3.2.1) if it's too late to re-open the 3.2 release?

@mraleph
Copy link
Member Author

mraleph commented Nov 14, 2023

@mit-mit I think it should only be affecting standalone dart on Mac OS X. I think nothing else is affected (e.g. Flutter targeting iOS & Mac OS X should work in debug and release).

That being said it is unclear why we would want to release something that is broken if we could still quickly patch it before release.

@mit-mit
Copy link
Member

mit-mit commented Nov 14, 2023

If we have time to patch it, then yes let's do that. But I would not postpone the planned releases if we're not able to patch.

@itsjustkevin
Copy link
Contributor

From the feedback here, it looks like patching in is the path, I’m going to mark this as approved.

@mraleph please submit, I’ll do my part and pass it on to the Flutter release eng team.

@itsjustkevin itsjustkevin added merge-to-stable cherry-pick-approved Label for approved cherrypick request labels Nov 14, 2023
copybara-service bot pushed a commit that referenced this issue Nov 14, 2023
This is follow up to bd57548 which switched us to use manual copying
for call back pages on Mac OS and iOS. However these newly allocated
pages need to be created with MAP_JIT flag otherwise OS will kill
us with code signing violation if hardened runtime is enabled.

This can only be observed when the binary is signed that's why
we have not seen it on CI.

TEST=manually signed and tested that it no longer crashes.

Fixes #53928

Changelog-Exempt: Changing base 3.2.0 release
Cherry-pick: https://dart-review.googlesource.com/c/sdk/+/333260
Cherry-pick-request: #54035
Change-Id: Ia096e4b6fe59a2a34bcea9e7501c289f831c1406
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/335881
Reviewed-by: Martin Kustermann <kustermann@google.com>
Commit-Queue: Slava Egorov <vegorov@google.com>
@sortie
Copy link
Contributor

sortie commented Nov 14, 2023

Sounds like y'all are on top of this. Just so you know how to react to these situations in general: Our infrastructure and procedures do support even last minute urgent patches prior to publication (which is the point of no return). It will in practice take a couple of hours to land the change and then the release has to be reauthored and rebuilt. It won't delay the release unless we're literally on the release day. There may be additional delays on the Flutter side that I wouldn't know about, which doesn't affect the standalone Dart release.

From a project point of view, those new stable artifacts are largely untested of course, so they introduce a bit of risk that the patch is somehow bad or has unintended consequences. That's the project policy tradeoff you just made, weighing well known breakage against potential unknown breakage, or whether to just postpone the release to do the testing. The systems can handle it all so you can always decide between these options :)

@itsjustkevin itsjustkevin added the cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. label Nov 14, 2023
@a-siva a-siva added the area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. label Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-vm Use area-vm for VM related issues, including code coverage, FFI, and the AOT and JIT backends. cherry-pick-approved Label for approved cherrypick request cherry-pick-merged Cherry-pick has been merged to the stable or beta branch. cherry-pick-review Issue that need cherry pick triage to approve merge-to-stable
Projects
None yet
Development

No branches or pull requests

7 participants