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

iOS (and other Apple platforms): the BCL has P/Invokes to libraries and/or functions that don't exist #47533

Closed
4 tasks done
rolfbjarne opened this issue Jan 27, 2021 · 15 comments
Assignees

Comments

@rolfbjarne
Copy link
Member

rolfbjarne commented Jan 27, 2021

Description

There are several DllImports in the BCL for iOS (and the other Apple platforms) that contain references to native functions that don't exist. It's highly preferable to not have any P/Invokes to native functions that don't exist, because it allows the AOT compiler to generate more efficient code. It also makes other performance improvements possible.

This is from the microsoft.netcore.app.runtime.ios-arm64 pack (6.0.0-alpha.1.21063.13).

Full output from custom tool I wrote: https://gist.github.com/rolfbjarne/b9a6f16d2a6697c0d8dfc46c39160404

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Security untriaged New issue has not been triaged by the area owner labels Jan 27, 2021
@ghost
Copy link

ghost commented Jan 27, 2021

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

There are several DllImports in the BCL that references to native functions that don't exist. It's highly preferable to not have any P/Invokes to native functions that don't exist, because it allows the AOT compiler to generate more efficient code. It also makes other performance improvements possible.

  • System.Diagnostics.Process.dl

    • Has a P/Invoke to SystemNative_ConfigureTerminalForChildProcess in libSystem.Native.dylib, but libSystem.Native.dylib doesn't provide that function.
  • System.Net.Quic.dll

    • Function: MsQuicOpen Library: libmsquic.dylib
  • System.Net.Sockets.dll

    • Function: ReadFile Library: kernel32.dll
  • System.Security.Cryptography.Algorithms.dll, System.Security.Cryptography.OpenSsl.dll and System.Security.Cryptography.X509Certificates.dll

    • Numerous references to functions in libSystem.Security.Cryptography.Native.Apple.dylib of the form AppleCryptoNative_* or CryptoNative_*. I believe this is because of pending work (issue #?)
  • System.Private.CoreLib.dll

This is from the microsoft.netcore.app.runtime.ios-arm64 pack (6.0.0-alpha.1.21063.13).

Full output from custom tool I wrote: https://gist.github.com/rolfbjarne/b9a6f16d2a6697c0d8dfc46c39160404

Author: rolfbjarne
Assignees: -
Labels:

area-System.Security, untriaged

Milestone: -

@bartonjs
Copy link
Member

System.Security.Cryptography.Algorithms.dll
    Library: libSystem.Security.Cryptography.Native.Apple.dylib Function: AppleCryptoNative_EccGenerateKey
        ❌ The function AppleCryptoNative_EccGenerateKey does not exist in libSystem.Security.Cryptography.Native.Apple.dylib

Yeah, functions like that one are conditional:

#if !defined(TARGET_IOS) && !defined(TARGET_TVOS)
int32_t AppleCryptoNative_EccGenerateKey(
int32_t keySizeBits, SecKeychainRef tempKeychain, SecKeyRef* pPublicKey, SecKeyRef* pPrivateKey, int32_t* pOSStatus)
{

Just means that the relevant Interop.cs is being included in the iOS build when it doesn't need to.

@joperezr joperezr added os-ios Apple iOS and removed untriaged New issue has not been triaged by the area owner labels Feb 9, 2021
@joperezr joperezr added this to the Future milestone Feb 9, 2021
@marek-safar marek-safar modified the milestones: Future, 6.0.0 Feb 18, 2021
@ghost ghost added the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 18, 2021
@marek-safar marek-safar removed the needs-further-triage Issue has been initially triaged, but needs deeper consideration or reconsideration label Feb 18, 2021
@filipnavara
Copy link
Member

Just means that the relevant Interop.cs is being included in the iOS build when it doesn't need to.

Actually it's worse because in some cases the Interop code is called on the managed side and fails in very unpredictable fashion. This was one reported instance on Discord:

Unhandled Exception:
System.EntryPointNotFoundException: AppleCryptoNative_SecKeychainItemCopyKeychain
   at Interop.AppleCrypto.SecKeychainItemCopyKeychain(IntPtr item)
   at System.Security.Cryptography.Apple.SafeTemporaryKeychainHandle.UntrackItem(IntPtr keychainItem)
   at System.Security.Cryptography.Apple.SafeKeychainItemHandle.ReleaseHandle()
   at System.Runtime.InteropServices.SafeHandle.InternalRelease(Boolean disposeOrFinalizeOperation)
   at System.Runtime.InteropServices.SafeHandle.Dispose(Boolean disposing)
   at System.Runtime.InteropServices.SafeHandle.Finalize()

filipnavara added a commit to filipnavara/xamarin-macios that referenced this issue Apr 9, 2021
…en native libraries are dynamically linked
filipnavara added a commit to filipnavara/xamarin-macios that referenced this issue Apr 9, 2021
…en native libraries are dynamically linked
filipnavara added a commit to filipnavara/xamarin-macios that referenced this issue Apr 9, 2021
…en native libraries are dynamically linked
spouliot pushed a commit to xamarin/xamarin-macios that referenced this issue Apr 10, 2021
…es (#11158)


* Workaround dotnet/runtime#47533 and avoid listing required symbols when native libraries are dynamically linked
* Don't call LibMonoNativeLinkMode on macOS builds
* Apply iOS workarounds also for tvOS and Mac Catalyst
@steveisok steveisok assigned MaximLipnin and unassigned akoeplinger Jun 15, 2021
@MaximLipnin
Copy link
Contributor

Regarding ReadFile pinvoke to kernel32.dll in System.Reflection.Metadata.dll - built for the apple mobile targets (e.g. using ./build.sh --arch x64 --os tvossimulator --configuration Release), this assembly doesn't contain any invocation of Interop.Kernel32.ReadFile

image

@steveisok
Copy link
Member

@rolfbjarne do you have any info on where you were getting the Readfile pinvoke error from?

@filipnavara
Copy link
Member

System.Reflection.Metadata had a ReadFile call in one code path. I believe @adamsitnik removed it in one of the refactorings but it may still affect older NuGets.

@adamsitnik
Copy link
Member

System.Reflection.Metadata had a ReadFile call in one code path. I believe @adamsitnik removed it in one of the refactorings but it may still affect older NuGets.

This is correct, I've removed it in #50367 for net6.0. We still have it for netstandard1.1 and netstandard2.0 where there is no Stream.Read(Span<byte>) which could be used instead

@rolfbjarne
Copy link
Member Author

rolfbjarne commented Jun 29, 2021

@rolfbjarne do you have any info on where you were getting the Readfile pinvoke error from?

I ran my script again (on 6.0.0-preview.7.21324.1), and it only reports issues in System.Private.CoreLib.dll (full results).

  • There are numerous P/Invokes to GlobalizationNative_* functions in a libSystem.Globalization.Native.dylib, but there's no libSystem.Globalization.Native.dylib.
  • Two functions from libhostpolicy.dylib (corehost_resolve_component_dependencies and corehost_set_error_writer), but there's no libhostpolicy.dylib.

@filipnavara
Copy link
Member

There are numerous P/Invokes to GlobalizationNative_* functions in a libSystem.Globalization.Native.dylib, but there's no libSystem.Globalization.Native.dylib.

That library is supposed to exist. Can you double check what is going on please? It may be a packaging issue.

Two functions from libhostpolicy.dylib (corehost_resolve_component_dependencies and corehost_set_error_writer), but there's no libhostpolicy.dylib.

Should be fixed by #54601.

@steveisok
Copy link
Member

There are numerous P/Invokes to GlobalizationNative_* functions in a libSystem.Globalization.Native.dylib, but there's no libSystem.Globalization.Native.dylib.

That library is supposed to exist. Can you double check what is going on please? It may be a packaging issue.

Yeah, that is strange.

Are we going to need to do something about ReadFile for older versions?

@rolfbjarne
Copy link
Member Author

Are we going to need to do something about ReadFile for older versions?

Not for now at least, if it becomes a problem we can have a look at it later.

@akoeplinger
Copy link
Member

akoeplinger commented Jun 29, 2021

libSystem.Globalization.Native.dylib not existing is expected, the globalization PAL lib is statically linked into the runtime now since #44505.

There's code in the runtime which hijacks the dllimport: https://github.com/dotnet/runtime/blob/main/src/mono/mono/metadata/native-library.c#L935-L941.

On wasm we use NO_GLOBALIZATION_SHIM to skip that code but I'm wondering how it works there since that doesn't change the dllimport itself. edit: wasm is statically linking all libraries and uses https://github.com/dotnet/runtime/blob/57bfe474518ab5b7cfe6bf7424a79ce3af9d6657/src/tasks/WasmAppBuilder/PInvokeTableGenerator.cs to patch up the dllimports

@filipnavara
Copy link
Member

Is there anything left to be done for this issue?

@steveisok
Copy link
Member

I don't think so. @rolfbjarne ?

@rolfbjarne
Copy link
Member Author

I don't think so either.

@steveisok steveisok removed this from the 6.0.0 milestone Jul 22, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

9 participants