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

[Android] Rename System.Security.Cryptography.Native.OpenSsl on Android #52406

Merged
merged 13 commits into from
May 14, 2021

Conversation

tqiu8
Copy link
Contributor

@tqiu8 tqiu8 commented May 6, 2021

Rename System.Security.Cryptography.Native.OpenSsl to System.Security.Cryptography.Native.Android
Fix #52327

@ghost
Copy link

ghost commented May 6, 2021

Tagging subscribers to this area: @directhex
See info in area-owners.md if you want to be subscribed.

Issue Details

Rename System.Security.Cryptography.Native.OpenSsl to System.Security.Cryptography.Native.Android
Fix #52327

Author: tqiu8
Assignees: -
Labels:

area-Infrastructure-mono

Milestone: -

Copy link
Member

@filipnavara filipnavara left a comment

Choose a reason for hiding this comment

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

Most of the places should actually remain unchanged since they reference the real OpenSSL shim library and not the Android one.

@tqiu8 tqiu8 requested a review from akoeplinger May 7, 2021 13:54
Copy link
Member

@akoeplinger akoeplinger left a comment

Choose a reason for hiding this comment

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

Looks good apart from two comments

@tqiu8 tqiu8 force-pushed the android-crypto-rename branch from 2b2ccf9 to 8db425d Compare May 11, 2021 20:13
@tqiu8 tqiu8 force-pushed the android-crypto-rename branch from 8db425d to d958ab7 Compare May 11, 2021 21:04
@akoeplinger
Copy link
Member

There are quite a lot of test failures with System.DllNotFoundException : libSystem.Security.Cryptography.Native.OpenSsl on the runtime-staging Android legs, e.g.

   at System.PlatformDetection.GetAlpnSupport() in /_/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs:line 162
   at System.Lazy`1[[System.Boolean, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ViaFactory(LazyThreadSafetyMode mode) in System.Private.CoreLib.dll:token 0x6000fc6+0x43
   at System.Lazy`1[[System.Boolean, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].ExecutionAndPublication(LazyHelper executionAndPublication, Boolean useDefaultConstructor) in System.Private.CoreLib.dll:token 0x6000fc7+0x22
   at System.Lazy`1[[System.Boolean, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].CreateValue() in System.Private.CoreLib.dll:token 0x6000fcc+0x74
   at System.Lazy`1[[System.Boolean, System.Private.CoreLib, Version=6.0.0.0, Culture=neutral, PublicKeyToken=7cec85d7bea7798e]].get_Value() in System.Private.CoreLib.dll:token 0x6000fd2+0xa
   at System.PlatformDetection.get_SupportsAlpn() in /_/src/libraries/Common/tests/TestUtilities/System/PlatformDetection.cs:line 168
   at System.Net.Test.Common.GenericLoopbackOptions..ctor() in /_/src/libraries/Common/tests/System/Net/Http/GenericLoopbackServer.cs:line 96
   at System.Net.Test.Common.LoopbackServer.Options..ctor() in /_/src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs:line 384
   at System.Net.Test.Common.LoopbackServer..ctor(Options options) in /_/src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs:line 29
   at System.Net.Test.Common.LoopbackServer.CreateServerAsync(Func`2 funcAsync, Options options) in /_/src/libraries/Common/tests/System/Net/Http/LoopbackServer.cs:line 70
   at System.Net.Tests.WebClientTestBase.DownloadFile_Success() in /_/src/libraries/System.Net.WebClient/tests/WebClientTest.cs:line 583
--- End of stack trace from previous location ---

This is because we include the Unix/OpenSSL version of Interop.CryptoNative.cs which has the wrong library name.

I think we should rename the CryptoNative field to AndroidCryptoNative for all the DllImports to AndroidCryptoNative_ functions to make these mistakes easier to catch.

@tqiu8
Copy link
Contributor Author

tqiu8 commented May 12, 2021

This is because we include the Unix/OpenSSL version of Interop.CryptoNative.cs which has the wrong library name.

I think we should rename the CryptoNative field to AndroidCryptoNative for all the DllImports to AndroidCryptoNative_ functions to make these mistakes easier to catch.

That was one of my original approaches, but Android still uses several Interop files in the src/libraries/Common/src/Interop/Unix directory (Dllimports into CryptoNative_), which would require splitting those files into the src/libraries/Common/src/Interop/Android directory as well, thus creating duplicate files. Renaming CryptoNative is more efficient in that it takes advantage of shared entrypoint names but is definitely more ambiguous and harder to catch. I suppose in the long run it's better to make the split between Android and Unix more distinct?

/cc @steveisok

@marek-safar
Copy link
Contributor

Can this be merged now?

@filipnavara
Copy link
Member

Can this be merged now?

I believe it is still broken and references a wrong name of the library in some of the managed code (per comments above and Helix results in the runtime-staging pipelines).

@tqiu8
Copy link
Contributor Author

tqiu8 commented May 14, 2021

From what I can tell, test failures are seemingly unrelated, re-running some of the failing lanes in case some of them were flakey.

@filipnavara
Copy link
Member

filipnavara commented May 14, 2021

FWIW it looks good to me. It would likely make sense to rename OpenSslCryptographicException to something Android specific but I don't think it's worth to hold up the PR because of it. It's a small detail and this PR would unblock any potential fixes of the name on the Xamarin side.

@steveisok
Copy link
Member

steveisok commented May 14, 2021

@tqiu8 It looks like you're hitting known flakes. The android arm64 failures are being tracked in dotnet/xharness#597 and the Android x86 failure is hitting #52649

@steveisok
Copy link
Member

/cc @grendello

@tqiu8 tqiu8 merged commit 6ddabaf into dotnet:main May 14, 2021
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
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.

Rename libSystem.Security.Cryptography.Native.OpenSsl on Android
6 participants