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

Drafted fix #25

Closed
wants to merge 1 commit into from
Closed

Drafted fix #25

wants to merge 1 commit into from

Conversation

maryamariyan
Copy link
Owner

@maryamariyan maryamariyan commented Jun 29, 2018

cc: @janvorli, @bartonjs

Proper fix for compilation issue caused by deprecated API in macOS Mojave by

using dlsym to call available API rather than suppressing deprecation warnings.

Fixes: #30599

@maryamariyan maryamariyan self-assigned this Jun 29, 2018
@maryamariyan maryamariyan force-pushed the up-mojave branch 3 times, most recently from db53bec to ed3262d Compare June 29, 2018 20:42
@maryamariyan maryamariyan changed the title drafted fix Drafted fix Jun 29, 2018

if (!checked)
{
secCertificateCopyKey = (SecKeyRef (*)(SecCertificateRef))dlsym(RTLD_DEFAULT, "SecCertificateCopyKey");
Copy link
Owner Author

Choose a reason for hiding this comment

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

@janvorli is there a way we could call dlsym only once in this code snippet?

{
secCertificateCopyKey = (SecKeyRef (*)(SecCertificateRef))dlsym(RTLD_DEFAULT, "SecCertificateCopyKey");
secCertificateCopyPublicKey = (OSStatus (*)(SecCertificateRef, SecKeyRef*))dlsym(RTLD_DEFAULT, "SecCertificateCopyPublicKey");
checked = 1;
Copy link

Choose a reason for hiding this comment

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

In C# I'd do this as Volatile.Write(ref checked, 1), or declare checked as volatile. Is declaring checked volatile here the right way to prevent a reordering race condition, @janvorli?

Copy link

Choose a reason for hiding this comment

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

(Or is my static local-fu weak, and we don't need to do a lazy initializer manually, because static locals guarantee that they only run their initializer once? (and therefore the two dlsym calls should move up to variable initializers))

@@ -42,7 +42,7 @@ Returns 1 on success, 0 on failure, any other value on invalid state.

Output:
pPublicKeyOut: Receives a CFRetain()ed SecKeyRef for the public key
pOSStatusOut: Receives the result of SecCertificateCopyPublicKey
pOSStatusOut: Receives the result of SecCertificateCopyKey
Copy link

Choose a reason for hiding this comment

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

Receives the result of SecCertificateCopyKey or SecCertificateCopyPublicKey, depending on the OS version.

@maryamariyan maryamariyan force-pushed the up-mojave branch 2 times, most recently from f12652e to 4327a05 Compare July 3, 2018 16:19
…jave by

using dlsym to call available API rather than suppressing deprecation warnings.

Fixes: #30599
@maryamariyan
Copy link
Owner Author

Closing this PR and opening dotnet#30815 on corefx fork instead

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.

Build on macOS Mojave DP fails
2 participants