From 24a7e810ac2889f735bba650a9052a395e998e86 Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Wed, 22 Feb 2023 22:01:50 +0100 Subject: [PATCH 1/4] [monodroid] Properly process satellite assemblies Fixes: https://github.com/xamarin/xamarin-android/issues/7819 Our native runtime uses a cache of pointers to loaded managed assembly images (essentially an array of the native struct `MonoImage` pointers) which is pre-allocated at build time and placed in the native `libxamarin-app.so` library. While generating the cache, we also generate hashes for a number of assembly name permutations (currently two per assembly - with and without the extension). Only unique assembly names are considered when generating the cache (it's possible to have duplicate names, because we package more than one copy of some assemblies - those which are architecture specific). This algorithm had a bug which made it ignore culture prefix in satellite assembly names (e.g. `en/MyAssembly.resources.dll`) and instead of several entries for each culture, we generated only two entries for `MyAssembly.resources.dll` and `MyAssembly.resources` but we still counted each culture-prefixed assembly and stored that number in `libxamarin-app.so` to be used at runtime to calculate number of entries in the cache. This made the array storing cached `MonoImage*` pointers to be smaller than the number of actual assemblies in the APK times 2 and in some cases we failed to look up pointer to some images and, as the result, passed a `NULL` pointer to MonoVM which then caused a segmentation fault trying to dereference the pointer. Stop ignoring the culture prefix for satellite assemblies in order to avoid the situation. Additionally, since the previous assumption that MonoVM will validate all pointers passed to its APIs turned out to be unwarranted, we now check more carefully for `NULL` pointers when trying to obtain a native function pointer from the MonoVM runtime. --- .../Tasks/GeneratePackageManagerJava.cs | 4 +++- .../MarshalMethodsNativeAssemblyGenerator.cs | 3 ++- src/monodroid/jni/xamarin-android-app-context.cc | 12 ++++-------- 3 files changed, 9 insertions(+), 10 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs index f9616ef190d..83594593445 100644 --- a/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs +++ b/src/Xamarin.Android.Build.Tasks/Tasks/GeneratePackageManagerJava.cs @@ -265,7 +265,9 @@ void AddEnvironment () HashSet archAssemblyNames = null; HashSet uniqueAssemblyNames = new HashSet (StringComparer.OrdinalIgnoreCase); Action updateAssemblyCount = (ITaskItem assembly) => { - string assemblyName = Path.GetFileName (assembly.ItemSpec); + // We need to use the 'RelativePath' metadata, if found, because it will give us the correct path for satellite assemblies - with the culture in the path. + string? relativePath = assembly.GetMetadata ("RelativePath"); + string assemblyName = String.IsNullOrEmpty (relativePath) ? Path.GetFileName (assembly.ItemSpec) : relativePath; if (!uniqueAssemblyNames.Contains (assemblyName)) { uniqueAssemblyNames.Add (assemblyName); } diff --git a/src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsNativeAssemblyGenerator.cs b/src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsNativeAssemblyGenerator.cs index 5c94e9b6e27..50aec79c9ed 100644 --- a/src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsNativeAssemblyGenerator.cs +++ b/src/Xamarin.Android.Build.Tasks/Utilities/MarshalMethodsNativeAssemblyGenerator.cs @@ -816,7 +816,8 @@ void WriteHashes () where T: struct uint index = 0; foreach (string name in uniqueAssemblyNames) { - string clippedName = Path.GetFileNameWithoutExtension (name); + // We must make sure we keep the possible culture prefix, which will be treated as "directory" path here + string clippedName = Path.Combine (Path.GetDirectoryName (name) ?? String.Empty, Path.GetFileNameWithoutExtension (name)); ulong hashFull = HashName (name, is64Bit); ulong hashClipped = HashName (clippedName, is64Bit); diff --git a/src/monodroid/jni/xamarin-android-app-context.cc b/src/monodroid/jni/xamarin-android-app-context.cc index 3654491b944..c1c401880e9 100644 --- a/src/monodroid/jni/xamarin-android-app-context.cc +++ b/src/monodroid/jni/xamarin-android-app-context.cc @@ -13,7 +13,7 @@ MonodroidRuntime::get_method_name (uint32_t mono_image_index, uint32_t method_to { uint64_t id = (static_cast(mono_image_index) << 32) | method_token; - log_debug (LOG_ASSEMBLY, "Looking for name of method with id 0x%llx, in mono image at index %u", id, mono_image_index); + log_debug (LOG_ASSEMBLY, "MM: looking for name of method with id 0x%llx, in mono image at index %u", id, mono_image_index); size_t i = 0; while (mm_method_names[i].id != 0) { if (mm_method_names[i].id == id) { @@ -58,19 +58,15 @@ MonodroidRuntime::get_function_pointer (uint32_t mono_image_index, uint32_t clas // We need to do that, as Mono APIs cannot be invoked from threads that aren't attached to the runtime. mono_thread_attach (mono_get_root_domain ()); - // We don't check for valid return values from image loader, class and method lookup because if any - // of them fails to find the requested entity, they will return `null`. In consequence, we can pass - // these pointers without checking all the way to `mono_method_get_unmanaged_callers_only_ftnptr`, after - // which call we check for errors. This saves some time (not much, but definitely more than zero) MonoImage *image = MonoImageLoader::get_from_index (mono_image_index); MarshalMethodsManagedClass &klass = marshal_methods_class_cache[class_index]; if (klass.klass == nullptr) { - klass.klass = mono_class_get (image, klass.token); + klass.klass = image != nullptr ? mono_class_get (image, klass.token) : nullptr; } - MonoMethod *method = mono_get_method (image, method_token, klass.klass); + MonoMethod *method = klass.klass != nullptr ? mono_get_method (image, method_token, klass.klass) : nullptr; MonoError error; - void *ret = mono_method_get_unmanaged_callers_only_ftnptr (method, &error); + void *ret = method != nullptr ? mono_method_get_unmanaged_callers_only_ftnptr (method, &error) : nullptr; if (XA_LIKELY (ret != nullptr)) { if constexpr (NeedsLocking) { From b475944a492a1186ece1ae086c1a2fddad75dbe9 Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Thu, 23 Feb 2023 18:53:33 +0100 Subject: [PATCH 2/4] Test(s) Add a test for the issue, based on the `MissingSatelliteAssemblyInLibrary` packaging test which, when it fails, will result in a `SIGABRT`: D monodroid-assembly: assembly_store_open_from_bundles: looking for bundled name: 'System.Private.CoreLib' (hash 0x6b0ff375198b9c17) F monodroid-assembly: Invalid assembly index 19, exceeds the maximum index of 11 F libc : Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 17957 (semblyinlibrary), pid 17957 (semblyinlibrary) Enhance the packaging test `MissingSatelliteAssemblyInLibrary` by adding more languages, to see if they're all packaged correctly. --- .../PackagingTest.cs | 17 +++++-- .../Tests/InstallAndRunTests.cs | 47 +++++++++++++++++++ 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/PackagingTest.cs b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/PackagingTest.cs index f69eeb8b5c1..ef3db27cad6 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/PackagingTest.cs +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.Android.Build.Tests/PackagingTest.cs @@ -807,12 +807,18 @@ public void MissingSatelliteAssemblyInLibrary () new BuildItem ("EmbeddedResource", "Foo.resx") { TextContent = () => InlineData.ResxWithContents ("Cancel") }, - new BuildItem ("EmbeddedResource", "Foo.es.resx") { - TextContent = () => InlineData.ResxWithContents ("Cancelar") - } } }; + var languages = new string[] {"es", "de", "fr", "he", "it", "pl", "pt", "ru", "sl" }; + foreach (string lang in languages) { + lib.OtherBuildItems.Add ( + new BuildItem ("EmbeddedResource", $"Foo.{lang}.resx") { + TextContent = () => InlineData.ResxWithContents ($"{lang}") + } + ); + } + var app = new XamarinAndroidApplicationProject { IsRelease = true, }; @@ -829,7 +835,10 @@ public void MissingSatelliteAssemblyInLibrary () var apk = Path.Combine (Root, appBuilder.ProjectDirectory, app.OutputPath, $"{app.PackageName}-Signed.apk"); var helper = new ArchiveAssemblyHelper (apk); - Assert.IsTrue (helper.Exists ($"assemblies/es/{lib.ProjectName}.resources.dll"), "Apk should contain satellite assemblies!"); + + foreach (string lang in languages) { + Assert.IsTrue (helper.Exists ($"assemblies/{lang}/{lib.ProjectName}.resources.dll"), $"Apk should contain satellite assembly for language '{lang}'!"); + } } } diff --git a/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs b/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs index f1ab011b0ee..c82c7cca196 100644 --- a/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs +++ b/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs @@ -28,6 +28,53 @@ public void Teardown () proj = null; } + [Test] + public void NativeAssemblyCacheWithSatelliteAssemblies () + { + var path = Path.Combine ("temp", TestName); + var lib = new XamarinAndroidLibraryProject { + ProjectName = "Localization", + OtherBuildItems = { + new BuildItem ("EmbeddedResource", "Foo.resx") { + TextContent = () => InlineData.ResxWithContents ("Cancel") + }, + } + }; + + var languages = new string[] {"es", "de", "fr", "he", "it", "pl", "pt", "ru", "sl" }; + foreach (string lang in languages) { + lib.OtherBuildItems.Add ( + new BuildItem ("EmbeddedResource", $"Foo.{lang}.resx") { + TextContent = () => InlineData.ResxWithContents ($"{lang}") + } + ); + } + + var app = new XamarinAndroidApplicationProject { + IsRelease = true, + }; + app.References.Add (new BuildItem.ProjectReference ($"..\\{lib.ProjectName}\\{lib.ProjectName}.csproj", lib.ProjectName, lib.ProjectGuid)); + app.SetAndroidSupportedAbis ("armeabi-v7a", "arm64-v8a", "x86", "x86_64"); + + using (var libBuilder = CreateDllBuilder (Path.Combine (path, lib.ProjectName))) + using (var appBuilder = CreateApkBuilder (Path.Combine (path, app.ProjectName))) { + Assert.IsTrue (libBuilder.Build (lib), "Library Build should have succeeded."); + Assert.IsTrue (appBuilder.Install (proj), "Install should have succeeded."); + + var apk = Path.Combine (Root, appBuilder.ProjectDirectory, + app.OutputPath, $"{app.PackageName}-Signed.apk"); + var helper = new ArchiveAssemblyHelper (apk); + + foreach (string lang in languages) { + Assert.IsTrue (helper.Exists ($"assemblies/{lang}/{lib.ProjectName}.resources.dll"), $"Apk should contain satellite assembly for language '{lang}'!"); + } + + Assert.True (appBuilder.RunTarget (proj, "_Run"), "Project should have run."); + Assert.True (WaitForActivityToStart (proj.PackageName, "MainActivity", + Path.Combine (Root, builder.ProjectDirectory, "logcat.log"), 30), "Activity should have started."); + } + } + [Test] public void GlobalLayoutEvent_ShouldRegisterAndFire_OnActivityLaunch ([Values (false, true)] bool isRelease) { From dab54dff336ca7957124c47345552e05d2f78abd Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Thu, 23 Feb 2023 21:21:32 +0100 Subject: [PATCH 3/4] Fix the new test --- .../Tests/InstallAndRunTests.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs b/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs index c82c7cca196..fb77d0d82e6 100644 --- a/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs +++ b/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs @@ -50,19 +50,18 @@ public void NativeAssemblyCacheWithSatelliteAssemblies () ); } - var app = new XamarinAndroidApplicationProject { + proj = new XamarinAndroidApplicationProject { IsRelease = true, }; - app.References.Add (new BuildItem.ProjectReference ($"..\\{lib.ProjectName}\\{lib.ProjectName}.csproj", lib.ProjectName, lib.ProjectGuid)); - app.SetAndroidSupportedAbis ("armeabi-v7a", "arm64-v8a", "x86", "x86_64"); + proj.References.Add (new BuildItem.ProjectReference ($"..\\{lib.ProjectName}\\{lib.ProjectName}.csproj", lib.ProjectName, lib.ProjectGuid)); + proj.SetAndroidSupportedAbis ("armeabi-v7a", "arm64-v8a", "x86", "x86_64"); using (var libBuilder = CreateDllBuilder (Path.Combine (path, lib.ProjectName))) - using (var appBuilder = CreateApkBuilder (Path.Combine (path, app.ProjectName))) { + using (var appBuilder = CreateApkBuilder (Path.Combine (path, proj.ProjectName))) { Assert.IsTrue (libBuilder.Build (lib), "Library Build should have succeeded."); Assert.IsTrue (appBuilder.Install (proj), "Install should have succeeded."); - var apk = Path.Combine (Root, appBuilder.ProjectDirectory, - app.OutputPath, $"{app.PackageName}-Signed.apk"); + var apk = Path.Combine (Root, appBuilder.ProjectDirectory, proj.OutputPath, $"{proj.PackageName}-Signed.apk"); var helper = new ArchiveAssemblyHelper (apk); foreach (string lang in languages) { From dde53ee887cafa123ab62f2f66d7142cf9f6c3f0 Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Mon, 27 Feb 2023 10:29:27 +0100 Subject: [PATCH 4/4] Use the `builder` field in the test Also, set `builder` to `null` after a test is done --- .../Tests/InstallAndRunTests.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs b/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs index fb77d0d82e6..8d46154edb4 100644 --- a/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs +++ b/tests/MSBuildDeviceIntegration/Tests/InstallAndRunTests.cs @@ -25,6 +25,7 @@ public void Teardown () Directory.Delete (builder.ProjectDirectory, recursive: true); builder?.Dispose (); + builder = null; proj = null; } @@ -56,19 +57,19 @@ public void NativeAssemblyCacheWithSatelliteAssemblies () proj.References.Add (new BuildItem.ProjectReference ($"..\\{lib.ProjectName}\\{lib.ProjectName}.csproj", lib.ProjectName, lib.ProjectGuid)); proj.SetAndroidSupportedAbis ("armeabi-v7a", "arm64-v8a", "x86", "x86_64"); - using (var libBuilder = CreateDllBuilder (Path.Combine (path, lib.ProjectName))) - using (var appBuilder = CreateApkBuilder (Path.Combine (path, proj.ProjectName))) { + using (var libBuilder = CreateDllBuilder (Path.Combine (path, lib.ProjectName))) { + builder = CreateApkBuilder (Path.Combine (path, proj.ProjectName)); Assert.IsTrue (libBuilder.Build (lib), "Library Build should have succeeded."); - Assert.IsTrue (appBuilder.Install (proj), "Install should have succeeded."); + Assert.IsTrue (builder.Install (proj), "Install should have succeeded."); - var apk = Path.Combine (Root, appBuilder.ProjectDirectory, proj.OutputPath, $"{proj.PackageName}-Signed.apk"); + var apk = Path.Combine (Root, builder.ProjectDirectory, proj.OutputPath, $"{proj.PackageName}-Signed.apk"); var helper = new ArchiveAssemblyHelper (apk); foreach (string lang in languages) { Assert.IsTrue (helper.Exists ($"assemblies/{lang}/{lib.ProjectName}.resources.dll"), $"Apk should contain satellite assembly for language '{lang}'!"); } - Assert.True (appBuilder.RunTarget (proj, "_Run"), "Project should have run."); + Assert.True (builder.RunTarget (proj, "_Run"), "Project should have run."); Assert.True (WaitForActivityToStart (proj.PackageName, "MainActivity", Path.Combine (Root, builder.ProjectDirectory, "logcat.log"), 30), "Activity should have started."); }