From 06b1d7f82143fb32b1397b3657de5b22fdcfdf7c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri, 2 Feb 2024 16:28:46 -0500 Subject: [PATCH] [monodroid] typemaps may need to load assemblies (#8625) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: https://github.com/xamarin/java.interop/commit/005c914170a0af9069ff18fd4dd9d45463dd5dc6 Context: https://github.com/xamarin/java.interop/pull/1181 Context: 25d1f007a7bd1ca3ce960315fabd04b9a687224e When attempting to bump to xamarin/java.interop@005c9141, multiple unit tests would fail, e.g. Java.Lang.LinkageError : net.dot.jni.test.CallVirtualFromConstructorDerived ----> System.NotSupportedException : Could not find System.Type corresponding to Java type JniTypeSignature(TypeName=net/dot/jni/test/CallVirtualFromConstructorDerived ArrayRank=0 Keyword=False) . This happened because xamarin/java.interop@005c9141 implicitly required that typemaps exist for `Java.Interop.JavaObject` subclasses. Fair enough; enter xamasrin/java.interop#1181, which added support to `Java.Interop.Tools.JavaCallableWrappers` to emit typemaps for `Java.Interop.JavaObject` subclasses. That caused *crashes* in `tests/Mono.Android-Tests`: E droid.NET_Test: JNI ERROR (app bug): accessed stale Local 0x75 (index 7 in a table of size 7) F droid.NET_Test: java_vm_ext.cc:570] JNI DETECTED ERROR IN APPLICATION: use of deleted local reference 0x75 … F droid.NET_Test: runtime.cc:630] native: #13 pc 00000000003ce865 /apex/com.android.runtime/lib64/libart.so (art::(anonymous namespace)::CheckJNI::GetObjectClass(_JNIEnv*, _jobject*)+837) The immediate cause of the crash was a "use after free" bug within `TypeManager.CreateInstance()` in a never-hit-before error path; the "use after free" bug was fixed in 25d1f007. However, the cause of the hitting a never-hit-before error path is because `EmbeddedAssemblies::typemap_java_to_managed()` would only map Java types to `System.Type` instances for assemblies that have *already been loaded*. If the assembly had not yet been loaded, then `EmbeddedAssemblies::typemap_java_to_managed()` would return `null`, and if the binding it couldn't find happens to be for `java.lang.Object`, we hit the (buggy!) "Where is the Java.Lang.Object wrapper" error condition. Commit 25d1f007 fixes that and a great many other related issues. What's left is `EmbeddedAssemblies::typemap_java_to_managed()`: it should *never* return null *unless* there is no typemap at all. Whether the target assembly has been loaded or not should be irrelevant. Update `EmbeddedAssemblies::typemap_java_to_managed()` so that it will load the target assembly if necessary. Additionally, before we figured out that we had a "use after free" bug, all we had to go on was that *something* related to `JNIEnv::GetObjectClass()` was involved. Review JNI usage around `JNIEnv::GetObjectClass()` and related invocations, and cleanup: * Simplify logic related to `JNIEnv::DeleteLocalRef()`. * Decrease scope of local variables. * Clear variables passed to `JNIEnv.DeleteLocalRef()`. Co-authored-by: Jonathan Pryor Co-authored-by: Marek Habersack --- src/Mono.Android/Java.Interop/TypeManager.cs | 12 +++++++--- src/monodroid/jni/embedded-assemblies.cc | 24 ++++++++++++++++++++ src/monodroid/jni/osbridge.cc | 9 +++----- 3 files changed, 36 insertions(+), 9 deletions(-) diff --git a/src/Mono.Android/Java.Interop/TypeManager.cs b/src/Mono.Android/Java.Interop/TypeManager.cs index 9d71981804f..c0993e1a9d9 100644 --- a/src/Mono.Android/Java.Interop/TypeManager.cs +++ b/src/Mono.Android/Java.Interop/TypeManager.cs @@ -267,7 +267,7 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership { Type? type = null; IntPtr class_ptr = JNIEnv.GetObjectClass (handle); - string class_name = GetClassName (class_ptr); + string? class_name = GetClassName (class_ptr); lock (TypeManagerMapDictionaries.AccessLock) { while (class_ptr != IntPtr.Zero && !TypeManagerMapDictionaries.JniToManaged.TryGetValue (class_name, out type)) { @@ -279,12 +279,18 @@ internal static IJavaPeerable CreateInstance (IntPtr handle, JniHandleOwnership IntPtr super_class_ptr = JNIEnv.GetSuperclass (class_ptr); JNIEnv.DeleteLocalRef (class_ptr); + class_name = null; class_ptr = super_class_ptr; - class_name = GetClassName (class_ptr); + if (class_ptr != IntPtr.Zero) { + class_name = GetClassName (class_ptr); + } } } - JNIEnv.DeleteLocalRef (class_ptr); + if (class_ptr != IntPtr.Zero) { + JNIEnv.DeleteLocalRef (class_ptr); + class_ptr = IntPtr.Zero; + } if (targetType != null && (type == null || diff --git a/src/monodroid/jni/embedded-assemblies.cc b/src/monodroid/jni/embedded-assemblies.cc index 82b7607f437..ac6fddfd36b 100644 --- a/src/monodroid/jni/embedded-assemblies.cc +++ b/src/monodroid/jni/embedded-assemblies.cc @@ -695,6 +695,30 @@ EmbeddedAssemblies::typemap_java_to_managed (hash_t hash, const MonoString *java if (module->image == nullptr) { module->image = mono_image_loaded (module->assembly_name); + + if (module->image == nullptr) { + log_debug (LOG_ASSEMBLY, "typemap: assembly '%s' hasn't been loaded yet, attempting a full load", module->assembly_name); + + // Fake a request from MonoVM to load the assembly. + MonoAssemblyName *assembly_name = mono_assembly_name_new (module->assembly_name); + MonoAssembly *assm; + + if (assembly_name == nullptr) { + log_error (LOG_ASSEMBLY, "typemap: failed to create Mono assembly name for '%s'", module->assembly_name); + assm = nullptr; + } else { + MonoAssemblyLoadContextGCHandle alc_gchandle = mono_alc_get_default_gchandle (); + MonoError mono_error; + assm = embeddedAssemblies.open_from_bundles (assembly_name, alc_gchandle, &mono_error, false /* ref_only */); + } + + if (assm == nullptr) { + log_warn (LOG_ASSEMBLY, "typemap: failed to load managed assembly '%s'", module->assembly_name); + } else { + module->image = mono_assembly_get_image (assm); + } + } + if (module->image == nullptr) { log_error (LOG_ASSEMBLY, "typemap: unable to load assembly '%s' when looking up managed type corresponding to Java type '%s'", module->assembly_name, to_utf8 (java_type_name).get ()); return nullptr; diff --git a/src/monodroid/jni/osbridge.cc b/src/monodroid/jni/osbridge.cc index 067e9019da4..f7ca9467243 100644 --- a/src/monodroid/jni/osbridge.cc +++ b/src/monodroid/jni/osbridge.cc @@ -657,15 +657,13 @@ OSBridge::add_reference_jobject (JNIEnv *env, jobject handle, jobject reffed_han java_class = env->GetObjectClass (handle); add_method_id = env->GetMethodID (java_class, "monodroidAddReference", "(Ljava/lang/Object;)V"); + env->DeleteLocalRef (java_class); if (add_method_id) { env->CallVoidMethod (handle, add_method_id, reffed_handle); - env->DeleteLocalRef (java_class); - return 1; } env->ExceptionClear (); - env->DeleteLocalRef (java_class); return 0; } @@ -910,7 +908,6 @@ OSBridge::gc_cleanup_after_java_collection (JNIEnv *env, int num_sccs, MonoGCBri MonoClass *klass; #endif MonoObject *obj; - jclass java_class; jobject jref; jmethodID clear_method_id; int i, j, total, alive, refs_added; @@ -942,8 +939,9 @@ OSBridge::gc_cleanup_after_java_collection (JNIEnv *env, int num_sccs, MonoGCBri sccs [i]->is_alive = 1; mono_field_get_value (obj, bridge_info->refs_added, &refs_added); if (refs_added) { - java_class = env->GetObjectClass (jref); + jclass java_class = env->GetObjectClass (jref); clear_method_id = env->GetMethodID (java_class, "monodroidClearReferences", "()V"); + env->DeleteLocalRef (java_class); if (clear_method_id) { env->CallVoidMethod (jref, clear_method_id); } else { @@ -957,7 +955,6 @@ OSBridge::gc_cleanup_after_java_collection (JNIEnv *env, int num_sccs, MonoGCBri } #endif } - env->DeleteLocalRef (java_class); } } else { abort_unless (!sccs [i]->is_alive, "Bridge SCC at index %d must NOT be alive", i);