Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent overlapped decompression of embedded assemblies #7732

Merged
merged 6 commits into from
Jan 26, 2023
Merged

Prevent overlapped decompression of embedded assemblies #7732

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9d2740d
Tentative fix for issue #7335, needs more testing
grendello Jan 23, 2023
171c170
Remove dead code
grendello Jan 24, 2023
8d11a8d
Update apkdesc files
grendello Jan 24, 2023
3b1c93b
Another batch of apkdesc updates
grendello Jan 24, 2023
232d8da
Merge branch 'main' into issue7335
grendello Jan 25, 2023
efc88b5
Fix after-merge SNAFU
grendello Jan 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions ...ild.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64SimpleDotNet.apkdesc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -35,7 +35,7 @@
"Size": 2265
},
"assemblies/UnnamedProject.dll": {
"Size": 3257
"Size": 3258
},
"classes.dex": {
"Size": 19020
Expand All @@ -44,7 +44,7 @@
"Size": 93552
},
"lib/arm64-v8a/libmonodroid.so": {
"Size": 434704
"Size": 379320
},
"lib/arm64-v8a/libmonosgen-2.0.so": {
"Size": 3089272
Expand All @@ -59,7 +59,7 @@
"Size": 152960
},
"lib/arm64-v8a/libxamarin-app.so": {
"Size": 16760
"Size": 16720
},
"META-INF/BNDLTOOL.RSA": {
"Size": 1213
Expand Down Expand Up @@ -95,5 +95,5 @@
"Size": 1904
}
},
"PackageSize": 2648394
"PackageSize": 2632010
}
12 changes: 6 additions & 6 deletions ...ild.Tasks/Tests/Xamarin.ProjectTools/Resources/Base/BuildReleaseArm64XFormsDotNet.apkdesc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -206,7 +206,7 @@
"Size": 93552
},
"lib/arm64-v8a/libmonodroid.so": {
"Size": 434704
"Size": 379320
},
"lib/arm64-v8a/libmonosgen-2.0.so": {
"Size": 3089272
Expand All @@ -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
Expand Down Expand Up @@ -1976,5 +1976,5 @@
"Size": 341228
}
},
"PackageSize": 7824132
"PackageSize": 7807748
}
53 changes: 27 additions & 26 deletions src/monodroid/jni/embedded-assemblies.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
{
Expand All @@ -81,31 +89,32 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb
if (header->magic == COMPRESSED_DATA_MAGIC) {
if (XA_UNLIKELY (compressed_assemblies.descriptors == nullptr)) {
log_fatal (LOG_ASSEMBLY, "Compressed assembly found but no descriptor defined");
exit (FATAL_EXIT_MISSING_ASSEMBLY);
abort ();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why abort() and not exit()? I forget what abort() does on Android…

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

exit() causes the application to restart, abort() kills it with a native SIGABRT stack trace

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are a lot of places that users complain about a "white screen", and it ended up their app was crashing and restarting.

Should we audit all of the exit() calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we should, I've been meaning to do it for quite a while now. I'll open a separate PR for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR #7734 opened

}
if (XA_UNLIKELY (header->descriptor_index >= compressed_assemblies.count)) {
log_fatal (LOG_ASSEMBLY, "Invalid compressed assembly descriptor index %u", header->descriptor_index);
exit (FATAL_EXIT_MISSING_ASSEMBLY);
abort ();
}

CompressedAssemblyDescriptor &cad = compressed_assemblies.descriptors[header->descriptor_index];
assembly_data_size = data_size - sizeof(CompressedAssemblyHeader);
if (!cad.loaded) {
if (XA_UNLIKELY (cad.data == nullptr)) {
log_fatal (LOG_ASSEMBLY, "Invalid compressed assembly descriptor at %u: no data", header->descriptor_index);
exit (FATAL_EXIT_MISSING_ASSEMBLY);
StartupAwareLock decompress_lock (assembly_decompress_mutex);

if (cad.loaded) {
set_assembly_data_and_size (reinterpret_cast<uint8_t*>(cad.data), cad.uncompressed_file_size, assembly_data, assembly_data_size);
return;
}
dellis1972 marked this conversation as resolved.
Show resolved Hide resolved

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 (XA_UNLIKELY (cad.data == nullptr)) {
log_fatal (LOG_ASSEMBLY, "Invalid compressed assembly descriptor at %u: no data", header->descriptor_index);
abort ();
}

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);
exit (FATAL_EXIT_MISSING_ASSEMBLY);
abort ();
} else {
log_debug (LOG_ASSEMBLY, "Compressed assembly '%s' is smaller than when the application was built. Adjusting accordingly.", name);
}
Expand All @@ -115,29 +124,23 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb
const char *data_start = reinterpret_cast<const char*>(data + sizeof(CompressedAssemblyHeader));
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));

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);
Comment on lines -119 to -120
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this timing message not that helpful?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this timing message not that helpful?

Yeah, I used it initially to see how much decompression itself costs us, but right now it's just wasted CPU cycles.

}

if (ret < 0) {
log_fatal (LOG_ASSEMBLY, "Decompression of assembly %s failed with code %d", name, ret);
exit (FATAL_EXIT_MISSING_ASSEMBLY);
abort ();
}

if (static_cast<uint64_t>(ret) != cad.uncompressed_file_size) {
log_debug (LOG_ASSEMBLY, "Decompression of assembly %s yielded a different size (expected %lu, got %u)", name, cad.uncompressed_file_size, static_cast<uint32_t>(ret));
exit (FATAL_EXIT_MISSING_ASSEMBLY);
abort ();
}
cad.loaded = true;
}
assembly_data = reinterpret_cast<uint8_t*>(cad.data);
assembly_data_size = cad.uncompressed_file_size;

set_assembly_data_and_size (reinterpret_cast<uint8_t*>(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);
}
}

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

std::string clipped_name;
clipped_name.assign (name.get (), len);

hash_t name_hash = xxhash::hash (name.get (), len);
log_debug (LOG_ASSEMBLY, "assembly_store_open_from_bundles: looking for bundled name: '%s' (hash 0x%zx)", name.get (), name_hash);

Expand Down Expand Up @@ -411,14 +411,15 @@ EmbeddedAssemblies::assembly_store_open_from_bundles (dynamic_local_string<SENSI

log_debug (
LOG_ASSEMBLY,
"Mapped: image_data == %p; debug_info_data == %p; config_data == %p; descriptor == %p; data size == %u; debug data size == %u; config data size == %u",
"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'",
assembly_runtime_info.image_data,
assembly_runtime_info.debug_info_data,
assembly_runtime_info.config_data,
assembly_runtime_info.descriptor,
assembly_runtime_info.descriptor->data_size,
assembly_runtime_info.descriptor->debug_data_size,
assembly_runtime_info.descriptor->config_data_size
assembly_runtime_info.descriptor->config_data_size,
name.get ()
);
}

Expand Down
8 changes: 5 additions & 3 deletions src/monodroid/jni/embedded-assemblies.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint8_t> const& buf, uint32_t num_entries, monodroid_should_register should_register, ZipEntryLoadState &state) noexcept;
Expand Down Expand Up @@ -333,6 +334,7 @@ namespace xamarin::android::internal {

AssemblyStoreHeader *index_assembly_store_header = nullptr;
AssemblyStoreHashEntry *assembly_store_hashes;
std::mutex assembly_decompress_mutex;
};
}

Expand Down