Handle NativeLibrary.GetExport/Free on libs not loaded through NativeLibrary.*Load* on Mono.

[lateralusX]

#47013 changed how kernel32.dll and Ws2_32.dll gets loaded on Windows. Instead of loading using NativeLibrary.Load these system libraries are now loaded directly using LoadLibraryEx, but symbols are still handled through NativeLibrary. This short-circuits some logic in Mono that assumes all libraries gets loaded through NativeLibrary.Load.

Fix adds ability to use OS handle in calls to NativeLibrary.GetExport/Free regardless if that handle was retrieved through a call to NativeLibrary.Load or not. Fix is not platform specific since the same scenarios can apply to all platforms.

[ghost]

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

Issue Details

---

#47013 changed how kernel32.dll and Ws2_32.dll gets loaded on Windows. Instead of loading using NativeLibrary.Load these system libraries are now loaded directly using LoadLibraryEx, but symbols are still handled through NativeLibrary. This short-circuits some logic in Mono that assumes all libraries gets loaded through NativeLibrary.Load.

Fix adds ability to use passed in HMODULE when not finding a matching library in our native library cache and use it directly in call to GetProcAddress on Windows, inline with CoreClr behavior.

Author:	lateralusX
Assignees:	-
Labels:	area-AssemblyLoader-mono
Milestone:	-

[lateralusX]

Windows failure needs investigation.

[CoffeeFlux]

This is a somewhat unfortunate problem, and I'm not sure a Windows-specific hack on top of NativeLibrary is the right fix—I think instead we should avoid intermixing direct platform calls and NativeLibrary.

In CoreCLR NativeLibrary is just a thin wrapper, but we can't do that in Mono because of the fallback handlers. I'd like to get rid of those and simplify the NativeLibrary logic (look for an email on deprecation soon!) but at least for now that's not possible.

[lateralusX]

This is a somewhat unfortunate problem, and I'm not sure a Windows-specific hack on top of NativeLibrary is the right fix—I think instead we should avoid intermixing direct platform calls and NativeLibrary.

In CoreCLR NativeLibrary is just a thin wrapper, but we can't do that in Mono because of the fallback handlers. I'd like to get rid of those and simplify the NativeLibrary logic (look for an email on deprecation soon!) but at least for now that's not possible.

Agree this is unfortunate, but right now I think we have a couple of options, change implementation in library to not use NativeLibrary.GetSymbol when you have loaded the library using direct calls to LoadLibrary(Ex), or enable option to call NativeLibrary.GetSymbol regardless how you loaded the library, what's implemented in this PR. We could also hook LoadLibrary(Ex) on Mono making sure we always get into our load library logic regardless how it is loaded, and third, we could add a list of pre-loaded system libraries always loaded at startup (and added into the native cache). The fix suggested in this PR is the least invasive one and if we plan to move towards a more thin wrapper in future, then this will be closer to where we are heading. Right now this breaks some low level functions on Windows, like creating System.DateTime on Mono so we will need a fix one way or another.

[jkotas]

In CoreCLR NativeLibrary is just a thin wrapper,

The native library IntPtr handle is documented to be the OS library handle. Number of code snippets out there depend on it, on both Windows and Unix, e.g.: #11901 (comment)

[lateralusX]

Yes and issue on Mono is that it has assumption that any IntPtr that is passed to NativeLibrary.GetExport is and OS library handle previously retrieve through NativeLibrary.*Load* method (adds library to an internal cache checked by NativeLibrary.GetExport).

But above pretty much says that we should be able to get symbols on all platforms using NativeLibrary.GetExport regardless how caller has get hold of its IntPtr OS handle, so then this fix could be transformed into a generic fix for all platforms and it can only be a thin wrapper on top of OS API's without any assumptions around how the OS library handle was initial retrieved.

[CoffeeFlux]

@jkotas yes, and we return the OS library handle. We just also have conversion logic inside that, like @lateralusX mentioned, assumed the handle was also created with NativeLibrary.

We cannot just directly look up the symbol (and in general can't be a naive wrapper around platform APIs) because that will bypass the fallback handlers, which some embedders including Xamarin products rely on. At whatever point we get rid of those, then we can simplify the internal NativeLibrary code. Until then, the conversion logic is necessary.

We could try calling the platform API on the OS handle if we fail to find a corresponding MonoDl, in which case this scenario will work correctly? I'm not a particularly big fan, and I think we should just discourage intermixing externally-loaded libraries with NativeLibrary, but that seems like the only option here if we don't want to take that stance. We can't get rid of the other NativeLibrary logic right now.

[lateralusX]

We could try calling the platform API on the OS handle if we fail to find a corresponding MonoDl, in which case this scenario will work correctly?

That's whats implemented in this PR, but we probably need to convert it to something working on all platforms, not just Windows, will change this PR to do that.

Guess the same assumption also applies to NativeLibrary.Free, that it needs to call underlying OS close method, regardless if we have the OS library handle in our cache or not.

[lambdageek]

So the issue is: Mono needs to associate extra info with the native module handle (in the case that an embedder is using hooks to modify native library lookups), but LoadLibraryEx will allow the managed code (CoreLib, in this case) to sidestep the embedder. (@CoffeeFlux this doesn't seem windows specific - if CoreLib had Interop.Unix.Dlopen it would be the same problem, right?)

On the other hand, we don't guarantee to the embedder that their hook is the only one installed, or that they will be called for everything (ie an earlier hook could resolve the module, and a later one won't be called - right?).

So we could (either logically, or actually) associate handles returned by LoadLibraryEx with a Windows-specific hook as if that Windows hook that happens to run before the embedder's hook:

• One way we could do that is by changing Interop.Kernel32.LoadLibraryEx from being a PInvoke to being a small wrapper around a PInvoke (or a qcall into mono to forward to LoadLibraryEx and also set up the association).

• another option is to treat an un-associated lib in NativeLibrary.GetSymbol as though it came from the LoadLibraryEx hook. (which btw is my one suggestion for this PR: in the !module case here, make subsequent netcore_handle_lookup(lib) calls return some pre-fabricated MonoDl instance that just calls GetProcAddress. That would also make it possible for NativeLibrary.Free do the right thing regardless of where the module handle came from.

[lateralusX]

Yes, that is what I'm leaning towards as well, return something that is a dummy MonoDl that will just make sure we can get the library handle passed to the implemented OS specific API's in mono_dl_symbol or mono_dl_close, I'm implementing something along those lines at the moment.

[CoffeeFlux]

This is indeed not Windows specific.

Creating a MonoDl in the failure case seems like a reasonable option to me.

One thing to note is that we currently handle the ref counting at the MonoDl level rather than relying on the OS APIs, so intermingling direct syscalls with NativeLibrary on the same library is always going to be a bug farm when it comes to opening/freeing. I am still inclined to say we should discourage that, but this will at least enable basic usage outside of freeing to work correctly.

[lateralusX]

I also see that we have a block list when we unload, so maybe the most safe way is to not mask it into the logic of that cache, but keep it in:

ves_icall_System_Runtime_InteropServices_NativeLibrary_GetSymbol
ves_icall_System_Runtime_InteropServices_NativeLibrary_FreeLib

when we fail to load library from cache, and use a dummy MonoDl just carrying the OS library handle as needed by direct calls to mono_dl_symbol/mono_dl_close.

Will make a proposal of that in this PR and see how it plays out.

[jkotas]

Note that there is also the other direction: Somebody may get the handle via NativeLibrary and pass it to the OS loader directly, e.g. to get versioned symbol via dlvsym.

[CoffeeFlux]

Yeah, and that should work - I think we even test that particular case. We return the OS handle directly, so passing it to other libraries to call into works fine.

[lateralusX]

Adjusted fix and PR description based on discussion.

[MichalStrehovsky]

Curious about the cache: on CoreCLR, this will print "Resolving" twice. Would this print Resolving just once in Mono because we're asking for the same symbol? Or am I speculating about the role of the cache too much?

using System;
using System.Reflection;
using System.Runtime.InteropServices;

NativeLibrary.SetDllImportResolver(Assembly.GetExecutingAssembly(), (_, _, _) =>
{
    Console.WriteLine("Resolving");
    return IntPtr.Zero;
});

MessageBoxW1(default, default, default, default);
MessageBoxW2(default, default, default, default);

[DllImport("user32", EntryPoint = "MessageBoxW", ExactSpelling = true)]
static extern int MessageBoxW1(IntPtr a, IntPtr b, IntPtr c, IntPtr d);

[DllImport("user32", EntryPoint = "MessageBoxW", ExactSpelling = true)]
static extern int MessageBoxW2(IntPtr a, IntPtr b, IntPtr c, IntPtr d);

[CoffeeFlux]

Not sure on that sample, but it's possible we cache more aggressively than CoreCLR on some of the loader callbacks. They're pretty expensive and that was a conscious choice.

[lateralusX]

Found issue failing Windows x64 libraries tests, unrelated to this fix, but this change made that issue to surface, due to missing COM support on Mono on Windows, more details here, #47759.

[lateralusX]

Looks like the Windows x64 test failures are fixed.

[lateralusX]

@lambdageek, @CoffeeFlux are you also OK with slightly changed solution?