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.

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

Replace all exit() calls with abort() in native code #7734

Merged
merged 4 commits into from
Jan 25, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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);
Copy link
Member

Choose a reason for hiding this comment

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

Are all of these FATAL_EXIT_* constants somewhere, where we could delete those, too?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're in JI, and one or two are used there and since JI runs on desktop, the use of exit() is fine I guess. @jonpryor?

Copy link
Member

Choose a reason for hiding this comment

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

the FATAL_EXIT_* constants are in xamarin/java.interop, some of which are used there.

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
6 changes: 3 additions & 3 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 @@ -474,7 +474,7 @@ Debug::process_cmd (int fd, char *cmd)
log_info (LOG_DEFAULT, "Debugger requested an exit, will exit immediately.\n");
fflush (stdout);
fflush (stderr);
exit (0);
Helpers::abort_application ();
Copy link
Member

Choose a reason for hiding this comment

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

This gives me pause; the original exit(0) is not signaling an error condition, so is calling abort_application() appropriate?

As this is part of the debugging infrastructure, I think it may be safer to keep this as exit(0), unless you want to do an end-to-end test to exercise this behavior and see what happens…

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem here is that the intention is to terminate the application
but exit doesn't do that while abort does.

From the pov of the debugger the application dies anyway
but from the pov of the developer, it will immediately restart itself
which is rather weird

}

bool use_fd = false;
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