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

enable GS Cookie on OSX #39143

Merged
merged 2 commits into from
Jul 13, 2020
Merged

enable GS Cookie on OSX #39143

merged 2 commits into from
Jul 13, 2020

Conversation

VSadov
Copy link
Member

@VSadov VSadov commented Jul 11, 2020

Use '#define READONLY_ATTR_ARGS section("__DATA,__const")' for the s_gsCookie

which puts it in read only data segment.
It used to be readonly TEXT segment and that was causing issues when container is the singlefile host executable.

Fixes:#38184

@VSadov VSadov changed the title [WIP] enable GS Cookie on OSX enable GS Cookie on OSX Jul 11, 2020
@VSadov VSadov requested a review from janvorli July 11, 2020 17:16
@VSadov VSadov marked this pull request as ready for review July 11, 2020 17:16
@@ -694,7 +694,7 @@ typedef DPTR(GSCookie) PTR_GSCookie;
#define READONLY_ATTR
#else
#ifdef __APPLE__
#define READONLY_ATTR_ARGS section("__TEXT,__const")
#define READONLY_ATTR_ARGS section("__DATA,__const")
Copy link
Member

Choose a reason for hiding this comment

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

Did you mean to change it only for CORECLR_EMBEDDED? See the previous discussion: #36808 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

@am11 do you see a reason to make this different for embedded and regular coreclr? It seems to me that it is fine to have it in the __DATA,__const segment for both.

Copy link
Member

Choose a reason for hiding this comment

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

If it is working for all cases, then sounds good. :)
I was just thinking that since we have __TEXT,__const _s_gsCookie in the osx binary from the beginning (and previous change did not alter the section type); considering the requirements such as Mojave hardened runtime, maybe we do not need to change the type for the existing (non-embedded) case.

Copy link
Member

Choose a reason for hiding this comment

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

I've verified that it works with the non-embedded coreclr with Mojave hardened runtime (running the app signed and from bundle) too.

Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@janvorli janvorli merged commit 371ae3c into dotnet:master Jul 13, 2020
@VSadov
Copy link
Member Author

VSadov commented Jul 13, 2020

Thanks!!

@VSadov VSadov deleted the enableGSCookieOSX branch July 13, 2020 17:03
@karelz karelz added this to the 5.0.0 milestone Aug 18, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants