Skip to content

Commit 6ef3d5c

Browse files
grendellojonpryor
authored andcommitted
[monodroid] Prevent overlapped decompression of embedded assemblies (dotnet#7732)
Fixes: dotnet#7335 Context: d236af5 Commit d236af5 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: #1 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: #2 pc 00000000002680bc /data/app/myapp.name-B9t_3dF9i8mDxJEKodZw5w==/split_config.arm64_v8a.apk!libmonosgen-2.0.so (offset 0x109b000) (BuildId: 4a5dd4396e8816b7f69881838bd549285213d53b) A/DEBUG: dotnet#3 pc 00000000002681e8 /data/app/myapp.name-B9t_3dF9i8mDxJEKodZw5w==/split_config.arm64_v8a.apk!libmonosgen-2.0.so (offset 0x109b000) (BuildId: 4a5dd4396e8816b7f69881838bd549285213d53b) A/DEBUG: dotnet#4 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.
1 parent 2afb81b commit 6ef3d5c

File tree

2 files changed

+26
-23
lines changed

2 files changed

+26
-23
lines changed

src/monodroid/jni/embedded-assemblies.cc

Lines changed: 21 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
#include "mono-image-loader.hh"
3636
#include "xamarin-app.hh"
3737
#include "cpp-util.hh"
38+
#include "monodroid-glue-internal.hh"
3839
#include "startup-aware-lock.hh"
3940
#include "timing-internal.hh"
4041
#include "search.hh"
@@ -73,6 +74,13 @@ void EmbeddedAssemblies::set_assemblies_prefix (const char *prefix)
7374
assemblies_prefix_override = prefix != nullptr ? utils.strdup_new (prefix) : nullptr;
7475
}
7576

77+
force_inline void
78+
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
79+
{
80+
dest_assembly_data = source_assembly_data;
81+
dest_assembly_data_size = source_assembly_data_size;
82+
}
83+
7684
force_inline void
7785
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
7886
{
@@ -91,17 +99,18 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb
9199
CompressedAssemblyDescriptor &cad = compressed_assemblies.descriptors[header->descriptor_index];
92100
assembly_data_size = data_size - sizeof(CompressedAssemblyHeader);
93101
if (!cad.loaded) {
102+
StartupAwareLock decompress_lock (assembly_decompress_mutex);
103+
104+
if (cad.loaded) {
105+
set_assembly_data_and_size (reinterpret_cast<uint8_t*>(cad.data), cad.uncompressed_file_size, assembly_data, assembly_data_size);
106+
return;
107+
}
108+
94109
if (XA_UNLIKELY (cad.data == nullptr)) {
95110
log_fatal (LOG_ASSEMBLY, "Invalid compressed assembly descriptor at %u: no data", header->descriptor_index);
96111
Helpers::abort_application ();
97112
}
98113

99-
bool log_timing = FastTiming::enabled () && !FastTiming::is_bare_mode ();
100-
size_t decompress_time_index;
101-
if (XA_UNLIKELY (log_timing)) {
102-
decompress_time_index = internal_timing->start_event (TimingEventKind::AssemblyDecompression);
103-
}
104-
105114
if (header->uncompressed_length != cad.uncompressed_file_size) {
106115
if (header->uncompressed_length > cad.uncompressed_file_size) {
107116
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
115124
const char *data_start = reinterpret_cast<const char*>(data + sizeof(CompressedAssemblyHeader));
116125
int ret = LZ4_decompress_safe (data_start, reinterpret_cast<char*>(cad.data), static_cast<int>(assembly_data_size), static_cast<int>(cad.uncompressed_file_size));
117126

118-
if (XA_UNLIKELY (log_timing)) {
119-
internal_timing->end_event (decompress_time_index, true /* uses_more_info */);
120-
internal_timing->add_more_info (decompress_time_index, name);
121-
}
122-
123127
if (ret < 0) {
124128
log_fatal (LOG_ASSEMBLY, "Decompression of assembly %s failed with code %d", name, ret);
125129
Helpers::abort_application ();
@@ -131,13 +135,12 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb
131135
}
132136
cad.loaded = true;
133137
}
134-
assembly_data = reinterpret_cast<uint8_t*>(cad.data);
135-
assembly_data_size = cad.uncompressed_file_size;
138+
139+
set_assembly_data_and_size (reinterpret_cast<uint8_t*>(cad.data), cad.uncompressed_file_size, assembly_data, assembly_data_size);
136140
} else
137141
#endif
138142
{
139-
assembly_data = data;
140-
assembly_data_size = data_size;
143+
set_assembly_data_and_size (data, data_size, assembly_data, assembly_data_size);
141144
}
142145
}
143146

@@ -357,9 +360,6 @@ EmbeddedAssemblies::assembly_store_open_from_bundles (dynamic_local_string<SENSI
357360
len -= sizeof(SharedConstants::DLL_EXTENSION) - 1;
358361
}
359362

360-
std::string clipped_name;
361-
clipped_name.assign (name.get (), len);
362-
363363
hash_t name_hash = xxhash::hash (name.get (), len);
364364
log_debug (LOG_ASSEMBLY, "assembly_store_open_from_bundles: looking for bundled name: '%s' (hash 0x%zx)", name.get (), name_hash);
365365

@@ -411,14 +411,15 @@ EmbeddedAssemblies::assembly_store_open_from_bundles (dynamic_local_string<SENSI
411411

412412
log_debug (
413413
LOG_ASSEMBLY,
414-
"Mapped: image_data == %p; debug_info_data == %p; config_data == %p; descriptor == %p; data size == %u; debug data size == %u; config data size == %u",
414+
"Mapped: image_data == %p; debug_info_data == %p; config_data == %p; descriptor == %p; data size == %u; debug data size == %u; config data size == %u; name == '%s'",
415415
assembly_runtime_info.image_data,
416416
assembly_runtime_info.debug_info_data,
417417
assembly_runtime_info.config_data,
418418
assembly_runtime_info.descriptor,
419419
assembly_runtime_info.descriptor->data_size,
420420
assembly_runtime_info.descriptor->debug_data_size,
421-
assembly_runtime_info.descriptor->config_data_size
421+
assembly_runtime_info.descriptor->config_data_size,
422+
name.get ()
422423
);
423424
}
424425

src/monodroid/jni/embedded-assemblies.hh

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -223,9 +223,10 @@ namespace xamarin::android::internal {
223223
#else // def NET
224224
static MonoAssembly* open_from_bundles_refonly (MonoAssemblyName *aname, char **assemblies_path, void *user_data);
225225
#endif // ndef NET
226-
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;
227-
static void get_assembly_data (XamarinAndroidBundledAssembly const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept;
228-
static void get_assembly_data (AssemblyStoreSingleAssemblyRuntimeData const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept;
226+
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;
227+
void get_assembly_data (uint8_t *data, uint32_t data_size, const char *name, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept;
228+
void get_assembly_data (XamarinAndroidBundledAssembly const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept;
229+
void get_assembly_data (AssemblyStoreSingleAssemblyRuntimeData const& e, uint8_t*& assembly_data, uint32_t& assembly_data_size) noexcept;
229230

230231
void zip_load_entries (int fd, const char *apk_name, monodroid_should_register should_register);
231232
void zip_load_individual_assembly_entries (std::vector<uint8_t> const& buf, uint32_t num_entries, monodroid_should_register should_register, ZipEntryLoadState &state) noexcept;
@@ -333,6 +334,7 @@ namespace xamarin::android::internal {
333334

334335
AssemblyStoreHeader *index_assembly_store_header = nullptr;
335336
AssemblyStoreHashEntry *assembly_store_hashes;
337+
std::mutex assembly_decompress_mutex;
336338
};
337339
}
338340

0 commit comments

Comments
 (0)