Work around an Android 9 dlopen bug on x86 #4914

grendello · 2020-07-13T15:54:09Z

This commit fixes an issue in Android 9 (API 28) 32-bit (ONLY) dynamic
loader issue where if the same library is loaded more than once, instead
of returning a handle to the already loaded copy, dlopen will
reinitialize the library thus resetting its state (BSS section is
cleared, global instances of C++ classes are re-instantiated etc).

When the library is reinitalized, however, our runtime is not - since
the Java Runtime.initialize, which ends up in our native code,
is not called because the library load is not part of the process
startup, when Runtime.initialize is invoked. This in turn leads to
situation when certain class members initialized in Runtime.initialize
and assumed to be correct throughout the application life are, in fact,
invalid and lead to crashes. In the instance of issue #4893, the
problem was that the MonodroidRuntime::Class_getName field was not
initialized to point to the Java.Lang.Class.getName Java method and
that, in turn, led to Xamarin.Android not being able to look up Java
types during application execution time.

libmonodroid.so is reloaded whenever Mono tries to resolve an
__Internal p/invoke (via a call to Xamarin.Android's
monodroid_dlopen fallback handler) - these p/invokes are assumed to
live in libmonodroid so we need to return a handle to it, so that Mono
can subsequently use dlsym to look up the required symbol. Given the
situation described in the previous paragraph, we need a way to not
reload libmonodroid or do something else to avoid it being
reinitialized.

Since there's no portable way to check whether a DSO is already
loaded we need to resort to "something else", implemented in this
commit. We move all the functions used in __Internal p/invokes to a
separate DSO which can be reloaded & reinitialized over and over again
without shredding our global state. This is done by way of introducing
a C++ interface class, MonoAndroidExternalAPI which is passed to the
new DSO on load time, thus giving it a way to invoke all the necessary
internal Xamarin.Android functions the __Internal p/invokes need in
order to work as expected.

jonathanpeppers · 2020-07-13T16:09:14Z

build-tools/installers/create-installers.targets

@@ -159,6 +159,7 @@
    <_MSBuildFiles Include="$(MSBuildSrcDir)\LayoutBinding.cs" />
    <_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmono-android.debug.so')"       Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />
    <_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmono-android.release.so')"     Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />
+    <_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmonodroid-mono-api.so')"       Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />


I think you need this new file in .NET 5+ packages:

Suggested change

<_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmonodroid-mono-api.so')" Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />

<_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmonodroid-mono-api.so')" />

Actually list it here, and leave this line as-is:

https://github.com/xamarin/xamarin-android/blob/b987d1b6bdbef4f1e858ed2c6d2f69504f614243/build-tools/create-packs/Microsoft.Android.Runtime.proj#L39-L42

@jonathanpeppers I mirrored the action for libmono-android*.so - the two don't work without each other, so is the change you suggested also required for libmono-android*.so?

jonpryor · 2020-07-13T17:04:39Z

build-tools/installers/create-installers.targets

@@ -159,6 +159,7 @@
    <_MSBuildFiles Include="$(MSBuildSrcDir)\LayoutBinding.cs" />
    <_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmono-android.debug.so')"       Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />
    <_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmono-android.release.so')"     Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />
+    <_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmonodroid-mono-api.so')"       Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />


Naming "quibble": we've been "trying" (kinda/sorta) to keep "monodroid" out of filenames, and nominally been successful:

% find /Library/Frameworks/Xamarin.Android.framework/Versions/Current -iname \*monodroid\* # No matches

(Within .apk naming is another matter…)

src/monodroid creates libmono-android* files, not libmonodroid* files.

I think we should continue this, and instead use libandroid-mono-api.so or something like that.

src/monodroid/jni/cpu-arch-detect.cc

jonpryor · 2020-07-13T17:07:31Z

src/monodroid/jni/external-api.cc

+static MonodroidExternalAPI *external_api = nullptr;
+
+MONO_API bool
+__init_external_api (MonodroidExternalAPI *api)


This is a name reserved by the compiler (even if something frequently used by others). Any particular reason for a double-underscore __ prefix and not _monodroid_?

src/monodroid/jni/monodroid-glue-external.hh

jonpryor · 2020-07-13T20:55:58Z

src/monodroid/jni/monodroid-glue.cc

+	if (!need_api_init)
+		return handle;
+
+	auto api = new MonodroidExternalAPI_Impl ();


I believe this is a possible memory leak: there's a new here, but no corresponding delete external_api anywhere in this PR.

If I understand the scenario correctly, every time we dlopen("libxa-mono-api.so"), Android will clear the memory used for static/global variables of that native lib (the BSS section), and without some form of _fini() in libxa-mono-api.so -- assuming Bionic even supports _init/_fini/__attribute((destructor))/etc. (See also the dlopen(3) man page?).

Without a way to free the instance that libxa-mono-api.so!external_api refers to, every subsequent dlopen("libxa-mono-api.so") will thus leak the MonodroidExternalAPI_Impl allocated from the previous dlopenI("libxa-mono-api.so") invocation.

A possible fix is to avoid new/delete, and instead use a global:

static MonodroidExternalAPI_Impl _monodroid_ext_api; void* MonodroidRuntime::monodroid_dlopen_log_and_return (…) { … if (!api_init (&_monodroid_ext_api)) { … } … }

This way we only ever create one instance of MonodroidExternalAPI_Impl, and it's never leaked.

This isn't a safe way. Yes, the bug is that the memory is cleared on this particular Android emulator image (and the scenario was that an armeabi app was running on an x86 emulator). But there's no guarantee that this will always be the case (in fact, the app I tested before our HelloWorld sample, did reload the DSO, just rechecked it, and return a different value of its handle) and, even though we don't have any state in the API class now, I'd rather leak a tiny amount of memory than risk sharing state with an unknown number of the DSO instances.

I will, however, investigate what bionic supports wrt destructors

How often is this codepath hit? How much memory could we possibly leak?

Are we hitting this codepath every time we invoke a P/Invoke to a __Internal method, in which case we could be hitting this code path thousands-millions of times, which could be a significant memory leak?

Or do we hit this code path every time we resolve a P/Invoke to a __Internal method, in which case we could at most leak dozens of instances, maybe? (Though each instance is probably on a separate code page, so that could still add up…)

What's our worst-case scenario here?

But there's no guarantee that this will always be the case…

I don't understand this response.

In the solution of this PR, we have two libraries in the .apk of interest:

libmonodroid.so

libxa-mono-api.so

We're setting things up such that we only ever load libmonodroid.so once, by using System.loadLibrary("monodroid") in the Java bootstrap code. All C/C++ dlopen() invocations, meanwhile, never load libmonodroid.so, but instead always load libxa-mono-api.so.

In this setup, the suggested _monodroid_ext_api would be loaded in libmonodroid.so. Thus, the object would only ever be created once, and would never move around in memory, and would never have it's BSS cleared.

The loading of libxa-mono-api.so, meanwhile, would itself contain only one pointer, which would be "reset" whenever we dlopen("libxa-mono-api.so") via our invocation of libxa-mono-api.so!_monodroid_init_external_api.

in fact, the app I tested before our HelloWorld sample, did reload the DSO, just rechecked it, and return a different value of its handle

Which DSO was reloaded? I'm assuming you mean libxa-mono-api.so, right? So what would it matter if the address returned by dlopen("libxa-mono-api.so") differs? We don't need to allocate a different MonodroidExternalAPI_Impl instance per libxa-mono-api.so loaded; all loaded libxa-mono-api.so libraries can all use the same instance, no?

I'd rather leak a tiny amount of memory than risk sharing state with an unknown number of the DSO instances.

Given the nature of things, I'm not sure that there's much risk to this sharing of state.

But there's no guarantee that this will always be the case…

I don't understand this response.

In the solution of this PR, we have two libraries in the .apk of interest:

* `libmonodroid.so` * `libxa-mono-api.so`

We're setting things up such that we only ever load libmonodroid.so once, by using System.loadLibrary("monodroid") in the Java bootstrap code. All C/C++ dlopen() invocations, meanwhile, never load libmonodroid.so, but instead always load libxa-mono-api.so.
In this setup, the suggested _monodroid_ext_api would be loaded in libmonodroid.so. Thus, the object would only ever be created once, and would never move around in memory, and would never have it's BSS cleared.
The loading of libxa-mono-api.so, meanwhile, would itself contain only one pointer, which would be "reset" whenever we dlopen("libxa-mono-api.so") via our invocation of libxa-mono-api.so!_monodroid_init_external_api.

Mono asks us to resolve __Internal, we dlopen the DSO, initialize it with the API, return the handle

The DSO somehow affects "state" of the API (e.g. one of the methods there needs to store some state and it's modified as a result of some call)

Mono asks us to resolve __Internal again (for instance for a different code path which hasn't been hit before), so we repeat 1. and 2.

Both instances created above are used by Mono at different times, stepping on each other's toes by modifying the state of the shared API class

in fact, the app I tested before our HelloWorld sample, did reload the DSO, just rechecked it, and return a different value of its handle

Which DSO was reloaded? I'm assuming you mean libxa-mono-api.so, right? So what would it matter if the address returned by dlopen("libxa-mono-api.so") differs? We don't need to allocate a different MonodroidExternalAPI_Impl instance per libxa-mono-api.so loaded; all loaded libxa-mono-api.so libraries can all use the same instance, no?

Yes, libxa-mono-api.so, since this is the "DSO" featured by this PR. If the address differs, then we have TWO or more copies of the assembly, all using a SINGLE instance of the API which may have some state. the fact it doesn't have anything now doesn't matter - in the future we may introduce some state and get bitten by the shared nature of the class if there are multiple instances of DSOs using it.

I'd rather leak a tiny amount of memory than risk sharing state with an unknown number of the DSO instances.

Given the nature of things, I'm not sure that there's much risk to this sharing of state.

I disagree, as replied above.

How often is this codepath hit? How much memory could we possibly leak?

Are we hitting this codepath every time we invoke a P/Invoke to a __Internal method, in which case we could be hitting this code path thousands-millions of times, which could be a significant memory leak?

Or do we hit this code path every time we resolve a P/Invoke to a __Internal method, in which case we could at most leak dozens of instances, maybe? (Though each instance is probably on a separate code page, so that could still add up…)

What's our worst-case scenario here?

From what I see, the __Internal DSO lookup is done once per assembly, so we'd leak N times where N equals the number of assemblies.

Next question is how much is leaked? Are we leaking a byte? A page? Multiple pages? Unfortunately, this is likely based on malloc(3) behavior, and I don't know offhand what it's likely to do. :-/

We "only" likely to have app in the low-hundreds of assemblies (scary, but pull in Forms & AndroidX & NuGet and…), so if it's only once per assembly, this could either be an ignorable amount (1 byte per allocation) or a shockingly large amount (1 or more pages per allocation).

Maybe a custom allocator would be appropriate?

I don't think it's once per every assembly - it's more likely per assembly which has any __Internal p/invokes which right now is two assemblies (?) in our collection. Size of the class is 8 bytes. Bionic uses jemalloc which will round up the allocation size to at least sizeof(double) and the allocated objects are packed closely together avoid wasting space (http://jemalloc.net/jemalloc.3.html - see the IMPLEMENTATION NOTES section) so I really think we shouldn't go overboard with custom allocator and that we don't have too much to worry about in this regard

jonpryor · 2020-07-14T00:11:27Z

src/monodroid/jni/monodroid-glue.cc

 	int dl_flags = monodroidRuntime.convert_dl_flags (flags);
 	bool libmonodroid_fallback = false;

-	/* name is nullptr when we're P/Invoking __Internal, so remap to libmonodroid */
+	/* name is nullptr when we're P/Invoking __Internal, so remap to libmonodroid-mono-api */


Comment is now out of date; needs to mention libxa-mono-api.so.

jonpryor · 2020-07-14T00:51:51Z

src/monodroid/jni/monodroid-glue-external.hh

+		static constexpr char INIT_FUNCTION_NAME[] = "_monodroid_init_external_api";
+
+	public:
+		virtual mono_bool monodroid_get_network_interface_up_state (const char *ifname, mono_bool *is_up) = 0;


Should this type have a public virtual destructor?

In theory, yes, in practice, no. It's a pure interface class, no code here ever runs. And the only descendant is our Impl class, from which no further classes can be derived.

Depending on how your "can we have a _fini function/__attribute__((destructor))-based cleanup code in libxa-internal-api.so?" investigation goes, we may want a virtual destructor, so that when libxa-internal-api.so!internal_calls is destructed/cleaned up, it'll do the correct thing.

For example, we could (maybe, perhaps, at least in spirit?) make internal_calls a std::unique_ptr<MonoAndroidInternalCalls>, and when the library is unloaded -- assuming it's unloaded or otherwise cleaned up! -- then the MonoAndroidInternalCalls destructor would be executed.

Needs more investigation, to be sure.

From a very quick scan of the Mono source, I don't think Mono calls dlclose on the handle it gets from us during runtime, it closes the handle(s) only as part of the app shutdown cleanup. So I'm not sure we'd gain anything from _fini or std::unique_ptr.

jonpryor · 2020-07-14T00:57:58Z

src/monodroid/jni/monodroid-glue-external.hh

+
+namespace xamarin::android
+{
+	class MonodroidExternalAPI


Naming quibble (?): the scenario being addressed is around P/Invokes to __Internal functions, in which mono eventually calls MonodroidRuntime::monodroid_dlopen(), which eventually calls dlopen("libxa-mono-api.so").

This entire codepath is around P/Invokes to __Internal. This is in no way "external". (Unless we're using "external" as a "mirror" to "internal", at which point my brain potentially starts hurting.)

Then we have PR #4757, in which -- to address .NET mono "hijacking" __Internal for its own use, preventing us from using it -- changes our P/Invokes to use monodroid instead of __Internal.

Wouldn't this just "reintroduce" the same problem, as a [DllImport("monodroid")] would still result in a subsequent monodroid_dlopen("monodroid"), which would clear BSS, and break everything?

Furthermore, the whole reason we can't merge #4757 yet is because by using [DllImport("monodroid")], we break desktop use, as there is no monodroid.dll/libmonodroid.dylib file for the Designer to load.

I thus suggest the following, in broad strokes:

"Merge" this PR & PR Stop using __Internal in our p/invoke calls #4757.

As part of the merging, instead of [DllImport("monodroid")], we do [DllImport("xa-internal-api")], which in turn means we need to build libxa-internal-api.dylib/etc for desktop platforms. (Or maybe xa-private, to "mirror" System.Private.CoreLib/etc.?)

libxa-mono-api.so is libxa-internal-api.so.
(On further thought, I'm not really sure why mono is in the library name, when most of the member function are for XA use, not mono…)

This type, MonodroidExternalAPI, should instead be MonoAndroidInternalCalls (kinda/sorta "mirroring" Mono.Android.dll).

Naming quibble (?): the scenario being addressed is around P/Invokes to __Internal functions, in which mono eventually calls MonodroidRuntime::monodroid_dlopen(), which eventually calls dlopen("libxa-mono-api.so").

This entire codepath is around P/Invokes to __Internal. This is in no way "external". (Unless we're using "external" as a "mirror" to "internal", at which point my brain potentially starts hurting.)

The __Internal name has been a misnomer forever and it's irrelevant here. The class is named External because it exposes external API functions from XA/Monodroid - the fact they are used for "internal" p/invokes is not relevant in this light IMO.

Then we have PR #4757, in which -- to address .NET mono "hijacking" __Internal for its own use, preventing us from using it -- changes our P/Invokes to use monodroid instead of __Internal.

Wouldn't this just "reintroduce" the same problem, as a [DllImport("monodroid")] would still result in a subsequent monodroid_dlopen("monodroid"), which would clear BSS, and break everything?

It would, and 4757 needs to be updated, obviously. This issue herewasn't known when #4757 was opened...

Furthermore, the whole reason we can't merge #4757 yet is because by using [DllImport("monodroid")], we break desktop use, as there is no monodroid.dll/libmonodroid.dylib file for the Designer to load.

While this PR builds libxa-mono-api.{dll,dylib}... So we should be able to update #4757 to use libxa-mono-api instead of monodroid

I thus suggest the following, in broad strokes:

1. "Merge" this PR & PR #4759

We can do that, but this PR fixes a real user issue while #4757 is for the world 12 months from now really. I'm fine with it if you think it's ok to merge them, though

2. As part of the merging, _instead of_ `[DllImport("monodroid")]`, we do `[DllImport("xa-internal-api")]`, which in turn means we need to _build_ `libxa-internal-api.dylib`/etc for desktop platforms. (Or maybe `xa-private`, to "mirror" `System.Private.CoreLib`/etc.?)

Already builds for desktop

3. `libxa-mono-api.so` _is_ `libxa-internal-api.so`. (On further thought, I'm not really sure why `mono` is in the library name, when _most_ of the member function are for XA use, not mono…)

It's not internal API as far as XA is concerned - it's an export of API calls, so it's by all means external - to be used by external code. However, I don't really care enough about the name, I'll just update it to internal in a while.

4. _This_ type, `MonodroidExternalAPI`, should instead be `MonoAndroidInternalCalls` (kinda/sorta "mirroring" `Mono.Android.dll`).

👍

"Merge" this PR & PR #4759

In retrospect, let's not do this. Instead, let's keep it in mind: the implicit intent here is that we shouldn't necessarily be P/Invoking to monodroid or __Internal, but instead we should eventually be P/Invoking to… (1) something that we control, and (2) a "real" filename, not a "virtual" one, which is the problem with monodroid & PR #4759: there is no "real" monodroid library unless you're on-device, which isn't the case for the Designer.

In this case, libxa-internal-api.so should be our preferred "stable" P/Invoke target.

jonpryor · 2020-07-14T01:02:08Z

src/monodroid/jni/external-api.cc

+	return true;
+}
+
+/* !DO NOT REMOVE! Used by Mono BCL (System.Net.NetworkInformation.NetworkInterface) */


Not relevant to this PR, but since I'm curious: how does the .NET 5 "hijacking" of __Internal mentioned in PR #4757 impact this function? How does __Internal work in .NET 5, in a way that we are even in the picture? (Are we in the picture?) What happens with System.Net.NetworkInformation.NetworkInterface?

I have no idea what is the answer here. I haven't investigated the .NET5 __Internal stuff yet, not even sure if it's already decide how it will work in the end. However, I think System.Net.NetworkInformation.NetworkInterface will not use our internal code but some dotnetcore equivalent.

grendello · 2020-07-16T07:51:38Z

/azp run

azure-pipelines · 2020-07-16T07:52:02Z

Azure Pipelines successfully started running 1 pipeline(s).

src/monodroid/CMakeLists.txt

jonpryor · 2020-07-16T23:42:07Z

src/monodroid/jni/internal-pinvoke-api.cc

+	va_end (args);
+}
+
+/* !DO NOT REMOVE! Used by libgdiplus.so */


How is this "used by libgdiplus.so"? Is this actually resolvable outside the "normal" __Internal mechanism?

(Do we even care? The "System.Drawing for Xamarin.Android" was never really shipped or used, was it?)

I have no idea if it was ever shipped. I did the porting and handed it over. I've just checked sources, it uses dlopen to load libmonodroid.so explicitly, so it won't work after this PR.

If it won't work after this PR, let's just remove the symbol entirely.

jonpryor · 2020-07-17T00:08:59Z

src/monodroid/jni/internal-pinvoke-api.cc

+	return internal_calls->monodroid_get_display_dpi (x_dpi, y_dpi);
+}
+
+MONO_API int


I'm not sure this is viable, which suggests we need a better "cross-reference mechanism". For example, set_assemblies_prefix() is from Embeddinator-4000:

https://github.com/mono/Embeddinator-4000/blob/d844acae5324fd6ac8eaf6c5c9c254b2b6a8a59e/support/mono_embeddinator.c#L56-L63

https://github.com/mono/Embeddinator-4000/blob/d844acae5324fd6ac8eaf6c5c9c254b2b6a8a59e/support/java/Runtime.java#L95

https://github.com/mono/Embeddinator-4000/blob/d844acae5324fd6ac8eaf6c5c9c254b2b6a8a59e/support/java/android/AndroidImpl.java#L26

It looks like Embeddinator-4000 implicitly requires that monodroid_embedded_assemblies_set_assemblies_prefix() be located in monodroid, because that's the only library it actually loads.

We really should use GitHub URLs, if at all possible, including commit hash. :-/

That said, a cursory investigation shows that Embeddinator-4000 has bit-rotten to the extent that it likely can't work with current xamarin-android -- monodroid can't be loaded until after libmonosgen-2.0.so is loaded, plus other dependencies! -- so perhaps it's not worth worrying about at all.

Which really proves that the only way for "Embeddinator-4000" to work reliably is for the core bits to be in xamarin-android, so that they won't get inadvertently broken.

Which I suppose is a long way of saying "let's just remove this symbol as well".

Isn't this also used by the Desginer?

I don't see any usage within xamarin/designer.

For me: https://github.com/search?q=monodroid_embedded_assemblies_set_assemblies_prefix&type=Code

All uses either appear to be from xamarin-android forks, or embeddinator clones. I think we can remove, or at least can consider removing.

What should be the process for removal, anyway? "Just remove"? Or update so that it logs a warning to logcat?

jonpryor · 2020-07-17T14:07:07Z

src/monodroid/jni/internal-pinvoke-api.cc

+}
+
+MONO_API char*
+monodroid_strdup_printf (const char *format, ...)


Silly Q, but why is this (and others?) exported in the first place? It's not used from Mono.Android.dll, and I can't find any uses of this symbol outside of src/monodroid anywhere on GitHub either: https://github.com/search?p=1&q=monodroid_strdup_printf&type=Code

I have no clue - I just moved things around, haven't checked if and where from they are still used.

A benefit to moving them all into one file is that they're a bit more obvious. That's nice.

jonpryor · 2020-07-17T14:08:19Z

src/monodroid/jni/internal-pinvoke-api.cc

+}
+
+MONO_API char**
+monodroid_strsplit (const char *str, const char *delimiter, size_t max_tokens)


Ditto this symbol.

grendello · 2020-07-20T09:15:57Z

/azp run

azure-pipelines · 2020-07-20T09:16:21Z

Azure Pipelines successfully started running 1 pipeline(s).

grendello · 2020-07-20T13:31:52Z

/azp run

Fixes: #4893 The setup: 1. App with `AndroidManifest.xml` which sets `/application/@android:extractNativeLibs` to `false`, e.g.: [assembly:Application (ExtractNativeLibs=false)] 2. Build (1) as a *Release* app 3. Run (2) on an Android 9.0 (API-28) *x86* emulator (32-bit!) The result: app crash during process startup! E/mono(14126): Unhandled Exception: E/mono(14126): System.ArgumentNullException: Value cannot be null. E/mono(14126): Parameter name: key E/mono(14126): at System.Collections.Generic.Dictionary`2[TKey,TValue].FindEntry (TKey key) E/mono(14126): at System.Collections.Generic.Dictionary`2[TKey,TValue].TryGetValue (TKey key, TValue& value) E/mono(14126): at Java.Interop.TypeManager.RegisterType (System.String java_class, System.Type t) E/mono(14126): at Android.Runtime.JNIEnv.RegisterJniNatives (System.IntPtr typeName_ptr, System.Int32 typeName_len, System.IntPtr jniClass, System.IntPtr methods_ptr, System.Int32 methods_len) The cause of the crash is an apparent Android bionic bug, a'la: void* h1 = dlopen ("…path/to/libmonodroid.so", RTLD_LAZY); // time passses void* h2 = dlopen ("…path/to/libmonodroid.so", RTLD_LAZY); "Normal"/expected semantics are that `h1` is the same as `h2`, with the second `dlopen()` invocation effectively being a no-op, as the library has already been loaded. On 32-bit Android 9 -- but *only* when `extractNativeLibs`=false! -- what *instead* happens is that that native library is *re-loaded* such that `h1` != `h2` (potentially?!), *and* the global state of `h1` is cleared; the library is reinitialized. Where this comes in is with P/Invokes to `__Internal`, which we "redirect" to `libmonodroid.so`. However, since `dlopen(NULL)` has a history on Android of generating a SIGSEGV, we avoid the crash by explicitly loading `libmonodroid.so` every time we need to satisfy a request for `__Internal` (in `MonodroidRuntime::monodroid_dlopen()` when `name` is `nullptr`). As we re-load `libmonodroid.so` and `dlopen()` potentially *clears* global data, what happens is that certain class members initialized in `Runtime.initInternal()` become `nullptr`. In this particular case, `MonodroidRuntime::Class_getName` became null, which meant that we couldn't lookup Java class names, which caused the subsequent crash. The fix is to introduce a new `libxa-internal-api.so` which contains only three things of consequence: 1. Explicit "init" and "shutdown" exports: MONO_API bool _monodroid_init_internal_api (MonoAndroidInternalCalls *api); MONO_API bool _monodroid_shutdown_internal_api (void); 2. A pointer to a new `MonoAndroidInternalCalls` "interface", implemented in `libmonodroid.so`, and 3. Other "C" exports that `__Internal` needs to invoke, which are implemented by delegating through `MonoAndroidInternalCalls`. `libxa-internal-api.so` thus becomes the *only* library that we ever `dlopen()` more than once, and as we control when it's loaded, we can call `_monodroid_init_internal_api()` and `_monodroid_shutdown_internal_api()` as appropriate, ensuring that when we run on Android 9+x86+`extractNativeLibs`=false, the "damage" from subsequent `dlopen()` invocations is minimized.

Context: dotnet#4914 Context: dotnet@d583b7c The .NET5 version of the Mono runtime "hijacked" `__Internal` for its own purposes and the change causes us trouble when trying to resolve p/invokes from our managed code to our runtime. Instead we now use `xa-internal-api` (added in d583b7c) to specify the p/invoke library name.

Context: #4914 Context: d583b7c Context: mono/mono#20295 (comment) Context? mono/mono#17369 Context? mono/mono@d5b6cf3 As a runtime extension, Mono long supported `[DllImport("__Internal")]` as a synonym for [`dlopen(NULL, 0)`][0]. Unfortunately, this [did not work on Android][1] -- Bionic would SIGSEGV when the filename parameter was `NULL` -- and as "fallout" of this change the `loader_func` that Xamarin.Android registers with mono via `mono_dl_fallback_register()` would be invoked *instead of* `dlopen(NULL)` when loading `__Internal`…but *only* on Android. Use of this feature remains widely used in Xamarin.Android, e.g. [`java-interop` is mapped to `__Internal`][2], avoiding the need to build and ship a separate `libjava-interop.so` library. Unfortunately, as part of .NET 5, MonoVM will be overhauling the semantics of `[DllImport("__Internal")]`, preventing `loader_func` from being invoked on Android to resolve `__Internal`. Consequently, we need to begin moving away from use of `__Internal`. Begin this process, and remove all P/Invokes to `__Internal` within `Mono.Android.dll` with P/Invokes to `xa-internal-api` (d583b7c). [0]: https://linux.die.net/man/3/dlopen [1]: mono/mono@9b73cd9 [2]: https://github.com/xamarin/xamarin-android/blob/faf1d16692ead1115d34fdf7b84c256c94a321da/src/monodroid/config.xml#L2

grendello requested review from jonpryor and radekdoulik July 13, 2020 15:54

grendello requested review from dellis1972 and jonathanpeppers as code owners July 13, 2020 15:54

jonathanpeppers reviewed Jul 13, 2020

View reviewed changes

grendello force-pushed the dlopen-bug-workaround branch from 0dd3d61 to 5983376 Compare July 13, 2020 16:50

jonpryor reviewed Jul 13, 2020

View reviewed changes

src/monodroid/jni/cpu-arch-detect.cc Show resolved Hide resolved

jonpryor reviewed Jul 13, 2020

View reviewed changes

grendello force-pushed the dlopen-bug-workaround branch from 5983376 to 1146621 Compare July 13, 2020 17:32

jonpryor reviewed Jul 13, 2020

View reviewed changes

src/monodroid/jni/monodroid-glue-external.hh Outdated Show resolved Hide resolved

jonpryor reviewed Jul 13, 2020

View reviewed changes

jonpryor reviewed Jul 14, 2020

View reviewed changes

grendello force-pushed the dlopen-bug-workaround branch 2 times, most recently from faa2b99 to b4e3380 Compare July 15, 2020 07:58

grendello force-pushed the dlopen-bug-workaround branch from b4e3380 to f58ae16 Compare July 16, 2020 12:41

jonpryor reviewed Jul 16, 2020

View reviewed changes

src/monodroid/CMakeLists.txt Show resolved Hide resolved

jonpryor reviewed Jul 16, 2020

View reviewed changes

jonpryor reviewed Jul 17, 2020

View reviewed changes

grendello force-pushed the dlopen-bug-workaround branch from f58ae16 to 70c0770 Compare July 17, 2020 16:18

jonpryor added 6 commits July 30, 2020 13:00

Fix whitespace

3bb1132

Add comments to #if

df747b5

Add comments to #if

e7eb434

Add comments to #if

4c99c4b

Add comments to #if

4769c7d

Missed one

7304d5f

jonpryor merged commit d583b7c into dotnet:master Jul 30, 2020

grendello deleted the dlopen-bug-workaround branch July 30, 2020 17:23

grendello mentioned this pull request Aug 18, 2020

Stop using __Internal in our p/invoke calls #4757

Merged

brendanzagaeski added this to the In Preview: 11.1 (d16-8) milestone Aug 26, 2020

github-actions bot locked and limited conversation to collaborators Jan 24, 2024

	<_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmonodroid-mono-api.so')" Condition=" '$(PackageId)' != 'Microsoft.Android.Sdk' " />
	<_MSBuildFiles Include="@(AndroidSupportedTargetJitAbi->'$(MSBuildSrcDir)\lib\%(Identity)\libmonodroid-mono-api.so')" />

Work around an Android 9 dlopen bug on x86 #4914

Work around an Android 9 dlopen bug on x86 #4914

Uh oh!

Conversation

grendello commented Jul 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

grendello commented Jul 16, 2020

Uh oh!

azure-pipelines bot commented Jul 16, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment