[MacCatalyst] Make AppleCryptoNative_SslSetEnabledCipherSuites check for 32 bit & 16 bit SSLCipherSuite #57623
Conversation
…for 32 bit & 16 bit SSLCipherSuite According to CipherSuites.h, SSLCipherSuite is a 16 bit value on iOS/tvOS x64 & arm64, but on MacCatalyst that is only true on arm64. x64 is defined as 32 bit. ``` /* 16-bit value on iOS */ typedef uint16_t SSLCipherSuite; /* 32-bit value elsewhere */ typedef uint32_t SSLCipherSuite; ``` Fixes dotnet#53120
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks Issue DetailsAccording to CipherSuites.h, SSLCipherSuite is a 16 bit value on iOS/tvOS x64 & arm64, but on MacCatalyst that is only true on arm64. x64 is defined as 32 bit. Fixes #53120
|
|
I'll have to check if Apple fixed the headers. At the time I filed the issue the Apple headers were actually incorrect for MacCatalyst arm64 and thus the check would still end up in incorrect branch. |
| @@ -596,19 +596,19 @@ int32_t AppleCryptoNative_SslSetEnabledCipherSuites(SSLContextRef sslContext, co | |||
| // Max numCipherSuites is 2^16 (all possible cipher suites) | |||
| assert(numCipherSuites < (1 << 16)); | |||
|
|
|||
| #if !defined(TARGET_MACCATALYST) && !defined(TARGET_IOS) && !defined(TARGET_TVOS) | |||
| #if !defined(TARGET_IOS) && !defined(TARGET_TVOS) | |||
There was a problem hiding this comment.
| #if !defined(TARGET_IOS) && !defined(TARGET_TVOS) | |
| #if !defined(TARGET_CPU_ARM64) && !defined(TARGET_IOS) && !defined(TARGET_TVOS) |
So, Apple headers are still broken and return sizeof(SSLCipherSuite) == sizeof(uint32_t) for MacCatalyst arm64. The simplest fix is to use the else (uint16_t) branch on any arm64 architecture.
Here's the additional rationale why the headers are broken: The system libraries that the app links to at runtime are identical for macOS and Mac Catalyst. Some of the APIs do runtime checks to adjust behavior but that's not the case for SSLSetEnabledCiphers. On macOS the API provably takes uint16_t and that's what is specified correctly in the system headers. For Mac Catalyst arm64 the SSLSetEnabledCiphers was already long obsolete by the time the arm64 architecture was added so Apple probably didn't bother with changing the type in the header.
There was a problem hiding this comment.
Sounds good though TARGET_CPU_ARM64 is coming from TargetConditionals.h and I'm not sure it's available here. We already have our own define TARGET_ARM64 that we're already using in other parts of the PAL so better to use that one.
There was a problem hiding this comment.
TargetConditionals.h is included but I am fine with either one as long as it works. I knew there was some other #define but I was not able to find where it is defined :-)
|
/backport to release/6.0-rc1 |
|
Started backporting to release/6.0-rc1: https://github.com/dotnet/runtime/actions/runs/1170882691 |
ghost
locked as resolved and limited conversation to collaborators
According to CipherSuites.h, SSLCipherSuite is a 16 bit value on iOS/tvOS x64 & arm64, but on MacCatalyst that is only true on arm64. x64 is defined as 32 bit.
Fixes #53120