From 2ff72d7c3331bd0a51da567846c3ccd95672b087 Mon Sep 17 00:00:00 2001 From: Marek Habersack Date: Thu, 26 Jan 2023 20:59:06 +0100 Subject: [PATCH] [monodroid] Prevent overlapped decompression of embedded assemblies (#7732) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes: https://github.com/xamarin/xamarin-android/issues/7335 Context: d236af54538ef1fe62d0125798cdde78e46dcd8e Commit d236af54 introduced embedded assembly compression, using LZ4, which speeds up startup and reduces final package size. Assemblies are compressed at build time and, at the same time, pre- allocated buffers for the **decompressed** data are allocated in `libxamarin-app.so`. The buffers are then passed to the LZ4 APIs, all threads using the same output buffer. The assumption was that we can do fine without locking as even if overlapped decompression happens, the output data will be the same and so even if two threads do the same thing at the same time, the data will be valid at all times, so long as at least one thread completes the decompression. This assumption proved to be **largely** true, but it appears that in high concurrency cases it is possible that the data in the decompression buffer differs. This can result in app crashes: A/libc: Fatal signal 6 (SIGABRT), code -1 (SI_QUEUE) in tid 3092 (.NET ThreadPool), pid 2727 (myapp.name) A/DEBUG: pid: 2727, tid: 3092, name: .NET ThreadPool >>> myapp.name <<< A/DEBUG: #01 pc 0000000000029b1c /data/app/myapp.name-B9t_3dF9i8mDxJEKodZw5w==/split_config.arm64_v8a.apk!libmono-android.release.so (offset 0x103d000) (xamarin::android::internal::MonodroidRuntime::mono_log_handler(char const*, char const*, char const*, int, void*)+144) (BuildId: 29c5a3805a0bedee1eede9b6668d7c676aa63371) A/DEBUG: #02 pc 00000000002680bc /data/app/myapp.name-B9t_3dF9i8mDxJEKodZw5w==/split_config.arm64_v8a.apk!libmonosgen-2.0.so (offset 0x109b000) (BuildId: 4a5dd4396e8816b7f69881838bd549285213d53b) A/DEBUG: #03 pc 00000000002681e8 /data/app/myapp.name-B9t_3dF9i8mDxJEKodZw5w==/split_config.arm64_v8a.apk!libmonosgen-2.0.so (offset 0x109b000) (BuildId: 4a5dd4396e8816b7f69881838bd549285213d53b) A/DEBUG: #04 pc 000000000008555c /data/app/myapp.name-B9t_3dF9i8mDxJEKodZw5w==/split_config.arm64_v8a.apk!libmonosgen-2.0.so (offset 0x109b000) (mono_metadata_string_heap+188) (BuildId: 4a5dd4396e8816b7f69881838bd549285213d53b) … My guess is that LZ4 either uses the output buffer as a scratchpad area when decompressing or that it initializes/modifies the buffer before writing actual data in it. With overlapped decompression, it may lead to one thread overwriting valid data previously written by another thread, so that when the latter returns the buffer it thought to have had valid data may contain certain bytes temporarily overwritten by the decompression session in the other, still running, thread. It may happen that MonoVM reads the corrupted data just when it is still invalid (before the still running decompression session actually writes the valid data), a classic race condition. To fix this, the decompression block is now protected with a startup- aware mutex. Mutex will be held only after the initial startup phase is completed, so there should not be much loss of startup performance. --- .../BuildReleaseArm64SimpleDotNet.apkdesc | 14 ++-- .../BuildReleaseArm64SimpleLegacy.apkdesc | 18 ++--- .../BuildReleaseArm64XFormsDotNet.apkdesc | 12 +-- .../BuildReleaseArm64XFormsLegacy.apkdesc | 76 +++++++++---------- src/monodroid/jni/embedded-assemblies.cc | 41 +++++----- src/monodroid/jni/embedded-assemblies.hh | 8 +- 6 files changed, 86 insertions(+), 83 deletions(-) diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.apkdesc b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.apkdesc index 36de188647a..1c9ddec5c6b 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.apkdesc +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.apkdesc @@ -8,13 +8,13 @@ "Size": 1024 }, "assemblies/Java.Interop.dll": { - "Size": 58916 + "Size": 58913 }, "assemblies/Mono.Android.dll": { - "Size": 87170 + "Size": 87106 }, "assemblies/Mono.Android.Runtime.dll": { - "Size": 5931 + "Size": 5860 }, "assemblies/rc.bin": { "Size": 1182 @@ -35,7 +35,7 @@ "Size": 2265 }, "assemblies/UnnamedProject.dll": { - "Size": 3257 + "Size": 3258 }, "classes.dex": { "Size": 19020 @@ -44,7 +44,7 @@ "Size": 93552 }, "lib/arm64-v8a/libmonodroid.so": { - "Size": 434704 + "Size": 379320 }, "lib/arm64-v8a/libmonosgen-2.0.so": { "Size": 3089272 @@ -59,7 +59,7 @@ "Size": 152960 }, "lib/arm64-v8a/libxamarin-app.so": { - "Size": 16760 + "Size": 16720 }, "META-INF/BNDLTOOL.RSA": { "Size": 1213 @@ -95,5 +95,5 @@ "Size": 1904 } }, - "PackageSize": 2648394 + "PackageSize": 2632010 } \ No newline at end of file diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleLegacy.apkdesc b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleLegacy.apkdesc index adfe30f0e3b..c865c13d9cd 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleLegacy.apkdesc +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleLegacy.apkdesc @@ -5,22 +5,22 @@ "Size": 2604 }, "assemblies/Java.Interop.dll": { - "Size": 68923 + "Size": 68913 }, "assemblies/Mono.Android.dll": { - "Size": 265140 + "Size": 265170 }, "assemblies/mscorlib.dll": { - "Size": 769036 + "Size": 769019 }, "assemblies/System.Core.dll": { - "Size": 28216 + "Size": 28199 }, "assemblies/System.dll": { - "Size": 9192 + "Size": 9180 }, "assemblies/UnnamedProject.dll": { - "Size": 2897 + "Size": 2881 }, "classes.dex": { "Size": 370828 @@ -32,7 +32,7 @@ "Size": 750976 }, "lib/arm64-v8a/libmonodroid.so": { - "Size": 333128 + "Size": 277744 }, "lib/arm64-v8a/libmonosgen-2.0.so": { "Size": 4039176 @@ -41,7 +41,7 @@ "Size": 66184 }, "lib/arm64-v8a/libxamarin-app.so": { - "Size": 21264 + "Size": 21256 }, "META-INF/ANDROIDD.RSA": { "Size": 1213 @@ -74,5 +74,5 @@ "Size": 1724 } }, - "PackageSize": 4003540 + "PackageSize": 3987156 } \ No newline at end of file diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsDotNet.apkdesc b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsDotNet.apkdesc index ab046791729..b8e223a8210 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsDotNet.apkdesc +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsDotNet.apkdesc @@ -11,13 +11,13 @@ "Size": 7313 }, "assemblies/Java.Interop.dll": { - "Size": 66790 + "Size": 66786 }, "assemblies/Mono.Android.dll": { - "Size": 444768 + "Size": 444701 }, "assemblies/Mono.Android.Runtime.dll": { - "Size": 5931 + "Size": 5860 }, "assemblies/mscorlib.dll": { "Size": 3863 @@ -206,7 +206,7 @@ "Size": 93552 }, "lib/arm64-v8a/libmonodroid.so": { - "Size": 434704 + "Size": 379320 }, "lib/arm64-v8a/libmonosgen-2.0.so": { "Size": 3089272 @@ -221,7 +221,7 @@ "Size": 152960 }, "lib/arm64-v8a/libxamarin-app.so": { - "Size": 333760 + "Size": 333720 }, "META-INF/android.support.design_material.version": { "Size": 12 @@ -1976,5 +1976,5 @@ "Size": 341228 } }, - "PackageSize": 7824132 + "PackageSize": 7807748 } \ No newline at end of file diff --git a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsLegacy.apkdesc b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsLegacy.apkdesc index 00393868d67..e1f72ba5de2 100644 --- a/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsLegacy.apkdesc +++ b/src/Xamarin.Android.Build.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsLegacy.apkdesc @@ -5,112 +5,112 @@ "Size": 3140 }, "assemblies/FormsViewGroup.dll": { - "Size": 7230 + "Size": 7215 }, "assemblies/Java.Interop.dll": { - "Size": 69966 + "Size": 69955 }, "assemblies/Mono.Android.dll": { - "Size": 572670 + "Size": 572710 }, "assemblies/Mono.Security.dll": { - "Size": 68449 + "Size": 68432 }, "assemblies/mscorlib.dll": { - "Size": 915425 + "Size": 915407 }, "assemblies/System.Core.dll": { - "Size": 164059 + "Size": 164046 }, "assemblies/System.dll": { - "Size": 388883 + "Size": 388865 }, "assemblies/System.Drawing.Common.dll": { - "Size": 12370 + "Size": 12365 }, "assemblies/System.Net.Http.dll": { - "Size": 110718 + "Size": 110693 }, "assemblies/System.Numerics.dll": { - "Size": 15706 + "Size": 15683 }, "assemblies/System.Runtime.Serialization.dll": { - "Size": 186683 + "Size": 186660 }, "assemblies/System.ServiceModel.Internals.dll": { - "Size": 26604 + "Size": 26593 }, "assemblies/System.Xml.dll": { - "Size": 395668 + "Size": 395657 }, "assemblies/UnnamedProject.dll": { - "Size": 116997 + "Size": 116986 }, "assemblies/Xamarin.AndroidX.Activity.dll": { - "Size": 7711 + "Size": 7697 }, "assemblies/Xamarin.AndroidX.AppCompat.AppCompatResources.dll": { - "Size": 6664 + "Size": 6648 }, "assemblies/Xamarin.AndroidX.AppCompat.dll": { - "Size": 125346 + "Size": 125328 }, "assemblies/Xamarin.AndroidX.CardView.dll": { - "Size": 7380 + "Size": 7366 }, "assemblies/Xamarin.AndroidX.CoordinatorLayout.dll": { - "Size": 18289 + "Size": 18272 }, "assemblies/Xamarin.AndroidX.Core.dll": { - "Size": 131944 + "Size": 131930 }, "assemblies/Xamarin.AndroidX.DrawerLayout.dll": { - "Size": 15443 + "Size": 15426 }, "assemblies/Xamarin.AndroidX.Fragment.dll": { - "Size": 43150 + "Size": 43135 }, "assemblies/Xamarin.AndroidX.Legacy.Support.Core.UI.dll": { - "Size": 6727 + "Size": 6715 }, "assemblies/Xamarin.AndroidX.Lifecycle.Common.dll": { - "Size": 7078 + "Size": 7062 }, "assemblies/Xamarin.AndroidX.Lifecycle.LiveData.Core.dll": { - "Size": 7208 + "Size": 7194 }, "assemblies/Xamarin.AndroidX.Lifecycle.ViewModel.dll": { - "Size": 4886 + "Size": 4873 }, "assemblies/Xamarin.AndroidX.Loader.dll": { - "Size": 13596 + "Size": 13585 }, "assemblies/Xamarin.AndroidX.RecyclerView.dll": { - "Size": 102349 + "Size": 102326 }, "assemblies/Xamarin.AndroidX.SavedState.dll": { - "Size": 6294 + "Size": 6268 }, "assemblies/Xamarin.AndroidX.SwipeRefreshLayout.dll": { - "Size": 11284 + "Size": 11272 }, "assemblies/Xamarin.AndroidX.ViewPager.dll": { - "Size": 19438 + "Size": 19424 }, "assemblies/Xamarin.Forms.Core.dll": { - "Size": 524743 + "Size": 524736 }, "assemblies/Xamarin.Forms.Platform.Android.dll": { - "Size": 384885 + "Size": 384872 }, "assemblies/Xamarin.Forms.Platform.dll": { "Size": 56878 }, "assemblies/Xamarin.Forms.Xaml.dll": { - "Size": 55807 + "Size": 55801 }, "assemblies/Xamarin.Google.Android.Material.dll": { - "Size": 43514 + "Size": 43497 }, "classes.dex": { "Size": 3533252 @@ -122,7 +122,7 @@ "Size": 750976 }, "lib/arm64-v8a/libmonodroid.so": { - "Size": 333128 + "Size": 277744 }, "lib/arm64-v8a/libmonosgen-2.0.so": { "Size": 4039176 @@ -131,7 +131,7 @@ "Size": 66184 }, "lib/arm64-v8a/libxamarin-app.so": { - "Size": 107032 + "Size": 107024 }, "META-INF/android.support.design_material.version": { "Size": 12 @@ -1883,5 +1883,5 @@ "Size": 341040 } }, - "PackageSize": 9537694 + "PackageSize": 9521310 } \ No newline at end of file diff --git a/src/monodroid/jni/embedded-assemblies.cc b/src/monodroid/jni/embedded-assemblies.cc index 3550435dc75..82b7607f437 100644 --- a/src/monodroid/jni/embedded-assemblies.cc +++ b/src/monodroid/jni/embedded-assemblies.cc @@ -35,6 +35,7 @@ #include "mono-image-loader.hh" #include "xamarin-app.hh" #include "cpp-util.hh" +#include "monodroid-glue-internal.hh" #include "startup-aware-lock.hh" #include "timing-internal.hh" #include "search.hh" @@ -73,6 +74,13 @@ void EmbeddedAssemblies::set_assemblies_prefix (const char *prefix) assemblies_prefix_override = prefix != nullptr ? utils.strdup_new (prefix) : nullptr; } +force_inline void +EmbeddedAssemblies::set_assembly_data_and_size (uint8_t* source_assembly_data, uint32_t source_assembly_data_size, uint8_t*& dest_assembly_data, uint32_t& dest_assembly_data_size) noexcept +{ + dest_assembly_data = source_assembly_data; + dest_assembly_data_size = source_assembly_data_size; +} + force_inline void EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[maybe_unused]] const char *name, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept { @@ -91,17 +99,18 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb CompressedAssemblyDescriptor &cad = compressed_assemblies.descriptors[header->descriptor_index]; assembly_data_size = data_size - sizeof(CompressedAssemblyHeader); if (!cad.loaded) { + StartupAwareLock decompress_lock (assembly_decompress_mutex); + + if (cad.loaded) { + set_assembly_data_and_size (reinterpret_cast(cad.data), cad.uncompressed_file_size, assembly_data, assembly_data_size); + return; + } + if (XA_UNLIKELY (cad.data == nullptr)) { log_fatal (LOG_ASSEMBLY, "Invalid compressed assembly descriptor at %u: no data", header->descriptor_index); Helpers::abort_application (); } - bool log_timing = FastTiming::enabled () && !FastTiming::is_bare_mode (); - size_t decompress_time_index; - if (XA_UNLIKELY (log_timing)) { - decompress_time_index = internal_timing->start_event (TimingEventKind::AssemblyDecompression); - } - if (header->uncompressed_length != cad.uncompressed_file_size) { if (header->uncompressed_length > cad.uncompressed_file_size) { log_fatal (LOG_ASSEMBLY, "Compressed assembly '%s' is larger than when the application was built (expected at most %u, got %u). Assemblies don't grow just like that!", name, cad.uncompressed_file_size, header->uncompressed_length); @@ -115,11 +124,6 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb const char *data_start = reinterpret_cast(data + sizeof(CompressedAssemblyHeader)); int ret = LZ4_decompress_safe (data_start, reinterpret_cast(cad.data), static_cast(assembly_data_size), static_cast(cad.uncompressed_file_size)); - if (XA_UNLIKELY (log_timing)) { - internal_timing->end_event (decompress_time_index, true /* uses_more_info */); - internal_timing->add_more_info (decompress_time_index, name); - } - if (ret < 0) { log_fatal (LOG_ASSEMBLY, "Decompression of assembly %s failed with code %d", name, ret); Helpers::abort_application (); @@ -131,13 +135,12 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb } cad.loaded = true; } - assembly_data = reinterpret_cast(cad.data); - assembly_data_size = cad.uncompressed_file_size; + + set_assembly_data_and_size (reinterpret_cast(cad.data), cad.uncompressed_file_size, assembly_data, assembly_data_size); } else #endif { - assembly_data = data; - assembly_data_size = data_size; + set_assembly_data_and_size (data, data_size, assembly_data, assembly_data_size); } } @@ -357,9 +360,6 @@ EmbeddedAssemblies::assembly_store_open_from_bundles (dynamic_local_stringdata_size, assembly_runtime_info.descriptor->debug_data_size, - assembly_runtime_info.descriptor->config_data_size + assembly_runtime_info.descriptor->config_data_size, + name.get () ); } diff --git a/src/monodroid/jni/embedded-assemblies.hh b/src/monodroid/jni/embedded-assemblies.hh index 58f12334e49..4b4c3324dae 100644 --- a/src/monodroid/jni/embedded-assemblies.hh +++ b/src/monodroid/jni/embedded-assemblies.hh @@ -223,9 +223,10 @@ namespace xamarin::android::internal { #else // def NET static MonoAssembly* open_from_bundles_refonly (MonoAssemblyName *aname, char **assemblies_path, void *user_data); #endif // ndef NET - static void get_assembly_data (uint8_t *data, uint32_t data_size, const char *name, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept; - static void get_assembly_data (XamarinAndroidBundledAssembly const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept; - static void get_assembly_data (AssemblyStoreSingleAssemblyRuntimeData const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept; + void set_assembly_data_and_size (uint8_t* source_assembly_data, uint32_t source_assembly_data_size, uint8_t*& dest_assembly_data, uint32_t& dest_assembly_data_size) noexcept; + void get_assembly_data (uint8_t *data, uint32_t data_size, const char *name, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept; + void get_assembly_data (XamarinAndroidBundledAssembly const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept; + void get_assembly_data (AssemblyStoreSingleAssemblyRuntimeData const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept; void zip_load_entries (int fd, const char *apk_name, monodroid_should_register should_register); void zip_load_individual_assembly_entries (std::vector const& buf, uint32_t num_entries, monodroid_should_register should_register, ZipEntryLoadState &state) noexcept; @@ -333,6 +334,7 @@ namespace xamarin::android::internal { AssemblyStoreHeader *index_assembly_store_header = nullptr; AssemblyStoreHashEntry *assembly_store_hashes; + std::mutex assembly_decompress_mutex; }; }