Skip to content

Commit

Permalink
[monodroid] Replace exit() with abort() in native code (dotnet#7734)
Browse files Browse the repository at this point in the history
Context: https://discord.com/channels/732297728826277939/732297837953679412/1067806732983746621
Context: https://discord.com/channels/732297728826277939/732297837953679412/1067822882274689024

In certain "bad app" scenarios, we would print a message and then
**exit**(3) the app.  The problem with using `exit()` in this manner
is that it *can* result in the app constantly relaunching, "spamming"
the UI.

Improve this by calling **abort**(3) instead.  This prevents Android
from constantly re-launching the app, and also prints useful abort
information such as registers and a native stack trace to `adb logcat`.

Unexpectedly, however, "simply" replacing app calls to `exit()` with
calls to `abort()` resulted in `libmonodroid.so` increasing in size
by ~16%.  For some reason, calling `abort()` caused the compiler to
inline much more code and in a weird manner (it repeated essentially
identical blocks of code from an inlined function, which were
previously folded into a single block when `exit()` was used).

We found that consolidating the `abort()` calls into
`Helpers::abort_application()` removed the size increase.
  • Loading branch information
grendello authored and jonpryor committed Jan 27, 2023
1 parent 52300df commit 0af6438
Show file tree
Hide file tree
Showing 17 changed files with 74 additions and 57 deletions.
2 changes: 2 additions & 0 deletions src/monodroid/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -486,6 +486,7 @@ set(XAMARIN_MONODROID_SOURCES
${SOURCES_DIR}/embedded-assemblies.cc
${SOURCES_DIR}/embedded-assemblies-zip.cc
${SOURCES_DIR}/globals.cc
${SOURCES_DIR}/helpers.cc
${SOURCES_DIR}/logger.cc
${SOURCES_DIR}/jni-remapping.cc
${SOURCES_DIR}/monodroid-glue.cc
Expand Down Expand Up @@ -564,6 +565,7 @@ set(XAMARIN_DEBUG_APP_HELPER_SOURCES
${SOURCES_DIR}/basic-utilities.cc
${SOURCES_DIR}/cpu-arch-detect.cc
${SOURCES_DIR}/debug-app-helper.cc
${SOURCES_DIR}/helpers.cc
${SOURCES_DIR}/new_delete.cc
${SOURCES_DIR}/shared-constants.cc
)
Expand Down
3 changes: 2 additions & 1 deletion src/monodroid/jni/cpp-util.hh
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include "cppcompat.hh"
#include "platform-compat.hh"
#include "helpers.hh"

static inline void
do_abort_unless (const char* fmt, ...)
Expand All @@ -32,7 +33,7 @@ do_abort_unless (const char* fmt, ...)
#endif // ndef ANDROID
va_end (ap);

std::abort ();
xamarin::android::Helpers::abort_application ();
}

#define abort_unless(_condition_, _fmt_, ...) \
Expand Down
4 changes: 3 additions & 1 deletion src/monodroid/jni/cxx-abi/terminate.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
#include <cstdlib>
#include <android/log.h>

#include "helpers.hh"

namespace std {
[[noreturn]] void
terminate () noexcept
{
__android_log_write (ANDROID_LOG_FATAL, "monodroid", "std::terminate() called. Aborting.");
abort ();
xamarin::android::Helpers::abort_application ();
}
}
4 changes: 2 additions & 2 deletions src/monodroid/jni/debug-app-helper.cc
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ Java_mono_android_DebugRuntime_init (JNIEnv *env, [[maybe_unused]] jclass klass,
void *monosgen = dlopen (monosgen_path, RTLD_LAZY | RTLD_GLOBAL);
if (monosgen == nullptr) {
log_fatal (LOG_DEFAULT, "Failed to dlopen Mono runtime from %s: %s", monosgen_path, dlerror ());
exit (FATAL_EXIT_CANNOT_FIND_LIBMONOSGEN);
Helpers::abort_application ();
}
}

Expand Down Expand Up @@ -276,7 +276,7 @@ get_libmonosgen_path ()

log_fatal (LOG_DEFAULT, "Do you have a shared runtime build of your app with AndroidManifest.xml android:minSdkVersion < 10 while running on a 64-bit Android 5.0 target? This combination is not supported.");
log_fatal (LOG_DEFAULT, "Please either set android:minSdkVersion >= 10 or use a build without the shared runtime (like default Release configuration).");
exit (FATAL_EXIT_CANNOT_FIND_LIBMONOSGEN);
Helpers::abort_application ();

return libmonoso;
}
Expand Down
4 changes: 2 additions & 2 deletions src/monodroid/jni/debug.cc
Original file line number Diff line number Diff line change
Expand Up @@ -248,7 +248,7 @@ Debug::start_debugging_and_profiling ()
DebuggerConnectionStatus res = start_connection (connect_args);
if (res == DebuggerConnectionStatus::Error) {
log_fatal (LOG_DEBUGGER, "Could not start a connection to the debugger with connection args '%s'.", connect_args);
exit (FATAL_EXIT_DEBUGGER_CONNECT);
Helpers::abort_application ();
} else if (res == DebuggerConnectionStatus::Connected) {
/* Wait for XS to configure debugging/profiling */
gettimeofday(&wait_tv, nullptr);
Expand Down Expand Up @@ -655,7 +655,7 @@ xamarin::android::conn_thread (void *arg)
res = instance->handle_server_connection ();
if (res && res != 3) {
log_fatal (LOG_DEBUGGER, "Error communicating with the IDE, exiting...");
exit (FATAL_EXIT_DEBUGGER_CONNECT);
Helpers::abort_application ();
}

return nullptr;
Expand Down
22 changes: 11 additions & 11 deletions src/monodroid/jni/embedded-assemblies-zip.cc
Original file line number Diff line number Diff line change
Expand Up @@ -47,12 +47,12 @@ EmbeddedAssemblies::zip_load_entry_common (size_t entry_index, std::vector<uint8
#endif
if (!result || entry_name.empty ()) {
log_fatal (LOG_ASSEMBLY, "Failed to read Central Directory info for entry %u in APK file %s", entry_index, state.apk_name);
exit (FATAL_EXIT_NO_ASSEMBLIES);
Helpers::abort_application ();
}

if (!zip_adjust_data_offset (state.apk_fd, state)) {
log_fatal (LOG_ASSEMBLY, "Failed to adjust data start offset for entry %u in APK file %s", entry_index, state.apk_name);
exit (FATAL_EXIT_NO_ASSEMBLIES);
Helpers::abort_application ();
}
#ifdef DEBUG
log_info (LOG_ASSEMBLY, " ZIP: local header offset: %u; data offset: %u; file size: %u", state.local_header_offset, state.data_offset, state.file_size);
Expand All @@ -79,7 +79,7 @@ EmbeddedAssemblies::zip_load_entry_common (size_t entry_index, std::vector<uint8
if ((state.data_offset & 0x3) != 0) {
log_fatal (LOG_ASSEMBLY, "Assembly '%s' is located at bad offset %lu within the .apk\n", entry_name.get (), state.data_offset);
log_fatal (LOG_ASSEMBLY, "You MUST run `zipalign` on %s\n", strrchr (state.apk_name, '/') + 1);
exit (FATAL_EXIT_MISSING_ZIPALIGN);
Helpers::abort_application ();
}

return true;
Expand Down Expand Up @@ -174,20 +174,20 @@ EmbeddedAssemblies::map_assembly_store (dynamic_local_string<SENSIBLE_PATH_MAX>
{
if (number_of_mapped_assembly_stores >= application_config.number_of_assembly_store_files) {
log_fatal (LOG_ASSEMBLY, "Too many assembly stores. Expected at most %u", application_config.number_of_assembly_store_files);
abort ();
Helpers::abort_application ();
}

md_mmap_info assembly_store_map = md_mmap_apk_file (state.apk_fd, state.data_offset, state.file_size, entry_name.get ());
auto header = static_cast<AssemblyStoreHeader*>(assembly_store_map.area);

if (header->magic != ASSEMBLY_STORE_MAGIC) {
log_fatal (LOG_ASSEMBLY, "Assembly store '%s' is not a valid Xamarin.Android assembly store file", entry_name.get ());
abort ();
Helpers::abort_application ();
}

if (header->version > ASSEMBLY_STORE_FORMAT_VERSION) {
log_fatal (LOG_ASSEMBLY, "Assembly store '%s' uses format v%u which is not understood by this version of Xamarin.Android", entry_name.get (), header->version);
abort ();
Helpers::abort_application ();
}

if (header->store_id >= application_config.number_of_assembly_store_files) {
Expand All @@ -198,13 +198,13 @@ EmbeddedAssemblies::map_assembly_store (dynamic_local_string<SENSIBLE_PATH_MAX>
header->store_id,
application_config.number_of_assembly_store_files
);
abort ();
Helpers::abort_application ();
}

AssemblyStoreRuntimeData &rd = assembly_stores[header->store_id];
if (rd.data_start != nullptr) {
log_fatal (LOG_ASSEMBLY, "Assembly store '%s' has a duplicate ID (%u)", entry_name.get (), header->store_id);
abort ();
Helpers::abort_application ();
}

constexpr size_t header_size = sizeof(AssemblyStoreHeader);
Expand Down Expand Up @@ -277,7 +277,7 @@ EmbeddedAssemblies::zip_load_entries (int fd, const char *apk_name, [[maybe_unus

if (!zip_read_cd_info (fd, cd_offset, cd_size, cd_entries)) {
log_fatal (LOG_ASSEMBLY, "Failed to read the EOCD record from APK file %s", apk_name);
exit (FATAL_EXIT_NO_ASSEMBLIES);
Helpers::abort_application ();
}
#ifdef DEBUG
log_info (LOG_ASSEMBLY, "Central directory offset: %u", cd_offset);
Expand All @@ -287,7 +287,7 @@ EmbeddedAssemblies::zip_load_entries (int fd, const char *apk_name, [[maybe_unus
off_t retval = ::lseek (fd, static_cast<off_t>(cd_offset), SEEK_SET);
if (retval < 0) {
log_fatal (LOG_ASSEMBLY, "Failed to seek to central directory position in the APK file %s. %s (result: %d; errno: %d)", apk_name, std::strerror (errno), retval, errno);
exit (FATAL_EXIT_NO_ASSEMBLIES);
Helpers::abort_application ();
}

std::vector<uint8_t> buf (cd_size);
Expand All @@ -306,7 +306,7 @@ EmbeddedAssemblies::zip_load_entries (int fd, const char *apk_name, [[maybe_unus
ssize_t nread = read (fd, buf.data (), static_cast<read_count_type>(buf.size ()));
if (static_cast<size_t>(nread) != cd_size) {
log_fatal (LOG_ASSEMBLY, "Failed to read Central Directory from the APK archive %s. %s (nread: %d; errno: %d)", apk_name, std::strerror (errno), nread, errno);
exit (FATAL_EXIT_NO_ASSEMBLIES);
Helpers::abort_application ();
}

if (application_config.have_assembly_store) {
Expand Down
26 changes: 13 additions & 13 deletions src/monodroid/jni/embedded-assemblies.cc
Original file line number Diff line number Diff line change
Expand Up @@ -81,19 +81,19 @@ 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);
Helpers::abort_application ();
}
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);
Helpers::abort_application ();
}

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);
Helpers::abort_application ();
}

bool log_timing = FastTiming::enabled () && !FastTiming::is_bare_mode ();
Expand All @@ -105,7 +105,7 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb
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);
Helpers::abort_application ();
} else {
log_debug (LOG_ASSEMBLY, "Compressed assembly '%s' is smaller than when the application was built. Adjusting accordingly.", name);
}
Expand All @@ -122,12 +122,12 @@ EmbeddedAssemblies::get_assembly_data (uint8_t *data, uint32_t data_size, [[mayb

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

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);
Helpers::abort_application ();
}
cad.loaded = true;
}
Expand Down Expand Up @@ -371,20 +371,20 @@ EmbeddedAssemblies::assembly_store_open_from_bundles (dynamic_local_string<SENSI

if (hash_entry->mapping_index >= application_config.number_of_assemblies_in_apk) {
log_fatal (LOG_ASSEMBLY, "Invalid assembly index %u, exceeds the maximum index of %u", hash_entry->mapping_index, application_config.number_of_assemblies_in_apk - 1);
abort ();
Helpers::abort_application ();
}

AssemblyStoreSingleAssemblyRuntimeData &assembly_runtime_info = assembly_store_bundled_assemblies[hash_entry->mapping_index];
if (assembly_runtime_info.image_data == nullptr) {
if (hash_entry->store_id >= application_config.number_of_assembly_store_files) {
log_fatal (LOG_ASSEMBLY, "Invalid assembly store ID %u, exceeds the maximum of %u", hash_entry->store_id, application_config.number_of_assembly_store_files - 1);
abort ();
Helpers::abort_application ();
}

AssemblyStoreRuntimeData &rd = assembly_stores[hash_entry->store_id];
if (hash_entry->local_store_index >= rd.assembly_count) {
log_fatal (LOG_ASSEMBLY, "Invalid index %u into local store assembly descriptor array", hash_entry->local_store_index);
abort ();
Helpers::abort_application ();
}

AssemblyStoreAssemblyDescriptor *bba = &rd.assemblies[hash_entry->local_store_index];
Expand Down Expand Up @@ -553,7 +553,7 @@ EmbeddedAssemblies::binary_search (const Key *key, const Entry *base, size_t nme
// This is a coding error on our part, crash!
if (base == nullptr) {
log_fatal (LOG_ASSEMBLY, "Map address not passed to binary_search");
exit (FATAL_EXIT_MISSING_ASSEMBLY);
Helpers::abort_application ();
}

[[maybe_unused]]
Expand Down Expand Up @@ -912,7 +912,7 @@ EmbeddedAssemblies::md_mmap_apk_file (int fd, uint32_t offset, size_t size, cons

if (mmap_info.area == MAP_FAILED) {
log_fatal (LOG_DEFAULT, "Could not `mmap` apk fd %d entry `%s`: %s", fd, filename, strerror (errno));
exit (FATAL_EXIT_CANNOT_FIND_APK);
Helpers::abort_application ();
}

mmap_info.size = offsetSize;
Expand All @@ -933,7 +933,7 @@ EmbeddedAssemblies::gather_bundled_assemblies_from_apk (const char* apk, monodro

if ((fd = open (apk, O_RDONLY)) < 0) {
log_error (LOG_DEFAULT, "ERROR: Unable to load application package %s.", apk);
exit (FATAL_EXIT_NO_ASSEMBLIES);
Helpers::abort_application ();
}
log_info (LOG_ASSEMBLY, "APK %s FD: %d", apk, fd);

Expand Down Expand Up @@ -1202,7 +1202,7 @@ EmbeddedAssemblies::try_load_typemaps_from_directory (const char *path)
std::unique_ptr<uint8_t[]> index_data = typemap_load_index (dir_fd, dir_path.get (), index_name);
if (!index_data) {
log_fatal (LOG_ASSEMBLY, "typemap: unable to load TypeMap data index from '%s/%s'", dir_path.get (), index_name);
exit (FATAL_EXIT_NO_ASSEMBLIES); // TODO: use a new error code here
Helpers::abort_application ();
}

for (size_t i = 0; i < type_map_count; i++) {
Expand Down
9 changes: 9 additions & 0 deletions src/monodroid/jni/helpers.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
#include "helpers.hh"

using namespace xamarin::android;

[[noreturn]] void
Helpers::abort_application () noexcept
{
std::abort ();
}
9 changes: 5 additions & 4 deletions src/monodroid/jni/helpers.hh
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
#define __HELPERS_HH

#include <cstdint>
#include <cstdlib>

#include "java-interop-util.h"
#include "platform-compat.hh"
Expand All @@ -21,8 +22,7 @@ namespace xamarin::android

if (XA_UNLIKELY (__builtin_add_overflow (a, b, &ret))) {
log_fatal (LOG_DEFAULT, "Integer overflow on addition at %s:%u", file, line);
exit (FATAL_EXIT_OUT_OF_MEMORY);
return static_cast<Ret>(0);
abort_application ();
}

return ret;
Expand All @@ -46,12 +46,13 @@ namespace xamarin::android

if (XA_UNLIKELY (__builtin_mul_overflow (a, b, &ret))) {
log_fatal (LOG_DEFAULT, "Integer overflow on multiplication at %s:%u", file, line);
exit (FATAL_EXIT_OUT_OF_MEMORY);
return static_cast<Ret>(0);
abort_application ();
}

return ret;
}

[[noreturn]] static void abort_application () noexcept;
};
}
#endif // __HELPERS_HH
2 changes: 1 addition & 1 deletion src/monodroid/jni/mono-log-adapter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ MonodroidRuntime::mono_log_handler (const char *log_domain, const char *log_leve

__android_log_write (prio, log_domain, message);
if (fatal) {
abort ();
Helpers::abort_application ();
}
}

Expand Down
Loading

0 comments on commit 0af6438

Please sign in to comment.