From d8a2f30c0c5103709824dc9cc5f8c29b20073097 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Mon, 21 Feb 2022 18:21:40 +0000 Subject: [PATCH 1/2] build(ndk): define version scripts to avoid exporting unneeded symbols reduce the surface of API, which expanded quite a bit with updated unwindstack. https://developer.android.com/ndk/guides/middleware-vendors debug builds export additional symbols used in the tests This change requires removal of code which re-throws types defined in binaries other than libbugsnag-ndk, because if the type has been hidden, then the throw invocation will trigger an immediate termination (skipping the crash report), or if the type is custom (e.g. a subclass of std::runtime_error, etc), then no useful message was detected anyway, as the type information is likely not available. Useful reading on the topic: * https://gcc.gnu.org/wiki/Visibility (specifically "Problems with C++ exceptions") * https://developer.android.com/ndk/guides/common-problems#rttiexceptions_not_working_across_library_boundaries * https://developer.android.com/ndk/guides/cpp-support#ic * https://en.cppreference.com/w/cpp/language/definition (section on ODR) Technically this lib should not export std:: symbols but this at least preserves existing behavior until its re-evaluated. [full ci] --- .../src/main/CMakeLists.txt | 5 ++ .../main/exported_native_symbols-Debug.txt | 13 +++++ ...exported_native_symbols-RelWithDebInfo.txt | 25 ++++++++++ .../src/main/jni/handlers/cpp_handler.cpp | 47 ------------------- features/smoke_tests/unhandled.feature | 2 +- features/steps/android_steps.rb | 7 +++ 6 files changed, 51 insertions(+), 48 deletions(-) create mode 100644 bugsnag-plugin-android-ndk/src/main/exported_native_symbols-Debug.txt create mode 100644 bugsnag-plugin-android-ndk/src/main/exported_native_symbols-RelWithDebInfo.txt diff --git a/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt b/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt index 2eec067ed9..c3c7f84a84 100644 --- a/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt +++ b/bugsnag-plugin-android-ndk/src/main/CMakeLists.txt @@ -40,9 +40,14 @@ target_link_libraries( # Specifies the target library. # Links the log library to the target library. log) +# Avoid exporting symbols in release mode to keep internals private +# More symbols are exported in debug mode for the sake of unit testing +set(EXTRA_LINK_FLAGS "-Wl,--version-script=${CMAKE_CURRENT_LIST_DIR}/exported_native_symbols-${CMAKE_BUILD_TYPE}.txt") + set_target_properties(bugsnag-ndk PROPERTIES COMPILE_OPTIONS -Werror -Wall -pedantic + LINK_FLAGS "${EXTRA_LINK_FLAGS}" CXX_STANDARD 17 CXX_STANDARD_REQUIRED YES) diff --git a/bugsnag-plugin-android-ndk/src/main/exported_native_symbols-Debug.txt b/bugsnag-plugin-android-ndk/src/main/exported_native_symbols-Debug.txt new file mode 100644 index 0000000000..90a7ad9ab0 --- /dev/null +++ b/bugsnag-plugin-android-ndk/src/main/exported_native_symbols-Debug.txt @@ -0,0 +1,13 @@ +LIBBUGSNAG_NDK { +global: + bugsnag_*; + Java_*; + bsg_*; + json_*; + BSG_MIGRATOR_CURRENT_VERSION; + migrate_app_v2; + __gxx_*; + __cxa_*; +local: + *; +}; diff --git a/bugsnag-plugin-android-ndk/src/main/exported_native_symbols-RelWithDebInfo.txt b/bugsnag-plugin-android-ndk/src/main/exported_native_symbols-RelWithDebInfo.txt new file mode 100644 index 0000000000..94204bd8fa --- /dev/null +++ b/bugsnag-plugin-android-ndk/src/main/exported_native_symbols-RelWithDebInfo.txt @@ -0,0 +1,25 @@ +LIBBUGSNAG_NDK { +global: + bugsnag_*; + __cxa_*; + __dynamic_cast; + __emutls_get_address; + __gxx_personality_v0; + Java_*; + extern "C++" { + "std::get_terminate()"; + "std::set_terminate(void (*)())"; + "std::set_unexpected(void (*)())"; + "std::get_new_handler()"; + "std::set_new_handler(void (*)())"; + "std::rethrow_exception(std::exception_ptr)"; + "std::__throw_bad_alloc()"; + "std::uncaught_exception()"; + "std::uncaught_exceptions()"; + "std::nothrow"; + "std::terminate()"; + }; + +local: + *; +}; diff --git a/bugsnag-plugin-android-ndk/src/main/jni/handlers/cpp_handler.cpp b/bugsnag-plugin-android-ndk/src/main/jni/handlers/cpp_handler.cpp index 268cdf0b88..61b1c80646 100644 --- a/bugsnag-plugin-android-ndk/src/main/jni/handlers/cpp_handler.cpp +++ b/bugsnag-plugin-android-ndk/src/main/jni/handlers/cpp_handler.cpp @@ -43,48 +43,6 @@ void bsg_handler_uninstall_cpp() { bsg_global_env = NULL; } -void bsg_write_current_exception_message(char *message, size_t length) { - try { - throw; - } catch (std::exception &exc) { - bsg_strncpy(message, (char *)exc.what(), length); - } catch (std::exception *exc) { - bsg_strncpy(message, (char *)exc->what(), length); - } catch (std::string obj) { - bsg_strncpy(message, (char *)obj.c_str(), length); - } catch (char *obj) { - snprintf(message, length, "%s", obj); - } catch (char obj) { - snprintf(message, length, "%c", obj); - } catch (short obj) { - snprintf(message, length, "%d", obj); - } catch (int obj) { - snprintf(message, length, "%d", obj); - } catch (long obj) { - snprintf(message, length, "%ld", obj); - } catch (long long obj) { - snprintf(message, length, "%lld", obj); - } catch (long double obj) { - snprintf(message, length, "%Lf", obj); - } catch (double obj) { - snprintf(message, length, "%f", obj); - } catch (float obj) { - snprintf(message, length, "%f", obj); - } catch (unsigned char obj) { - snprintf(message, length, "%u", obj); - } catch (unsigned short obj) { - snprintf(message, length, "%u", obj); - } catch (unsigned int obj) { - snprintf(message, length, "%u", obj); - } catch (unsigned long obj) { - snprintf(message, length, "%lu", obj); - } catch (unsigned long long obj) { - snprintf(message, length, "%llu", obj); - } catch (...) { - // no way to describe what this is - } -} - void bsg_handle_cpp_terminate() { if (bsg_global_env == NULL || bsg_global_env->handling_crash) return; @@ -108,11 +66,6 @@ void bsg_handle_cpp_terminate() { (char *)tinfo->name(), sizeof(bsg_global_env->next_event.error.errorClass)); } - size_t message_length = sizeof(bsg_global_env->next_event.error.errorMessage); - char message[message_length]; - bsg_write_current_exception_message(message, message_length); - bsg_strncpy(bsg_global_env->next_event.error.errorMessage, (char *)message, - message_length); if (bsg_run_on_error()) { bsg_increment_unhandled_count(&bsg_global_env->next_event); diff --git a/features/smoke_tests/unhandled.feature b/features/smoke_tests/unhandled.feature index 4d06fdfbde..5a7bc701d6 100644 --- a/features/smoke_tests/unhandled.feature +++ b/features/smoke_tests/unhandled.feature @@ -190,7 +190,7 @@ Scenario: C++ exception thrown with overwritten config # Exception details And the error payload field "events" is an array with 1 elements - And the exception "message" equals "How about NO" + And the exception "errorClass" demangles to "std::runtime_error*" And the exception "type" equals "c" And the event "unhandled" is true And the event "severity" equals "error" diff --git a/features/steps/android_steps.rb b/features/steps/android_steps.rb index 95c0c87153..a76c97201a 100644 --- a/features/steps/android_steps.rb +++ b/features/steps/android_steps.rb @@ -98,6 +98,13 @@ Maze.check.include(possible_values.raw.flatten, value) end +Then("the exception {string} demangles to {string}") do |keypath, expected_value| + actual_value = Maze::Helper.read_key_path(Maze::Server.errors.current[:body], "events.0.exceptions.0.#{keypath}") + demangled_value = `c++filt --types --no-strip-underscore '#{actual_value}'`.chomp + Maze.check.true(demangled_value == expected_value, + "expected '#{demangled_value}' to equal '#{expected_value}'") +end + # Checks whether the first significant frames match several given frames # # @param expected_values [Array] A table dictating the expected files and methods of the frames From d84ec76c15a8f7531c3ff5c400da758c1d831201 Mon Sep 17 00:00:00 2001 From: Delisa Mason Date: Wed, 23 Feb 2022 10:48:43 +0000 Subject: [PATCH 2/2] test(ndk): assert stack contents in c++ exception scenario --- .../cxx-scenarios-bugsnag/CMakeLists.txt | 3 +- .../main/cpp/CXXExceptionSmokeScenario.cpp | 29 +++++++++++++++++++ .../src/main/cpp/cxx-scenarios-bugsnag.cpp | 10 ------- features/smoke_tests/unhandled.feature | 13 ++++----- features/steps/android_steps.rb | 3 +- 5 files changed, 39 insertions(+), 19 deletions(-) create mode 100644 features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/CXXExceptionSmokeScenario.cpp diff --git a/features/fixtures/mazerunner/cxx-scenarios-bugsnag/CMakeLists.txt b/features/fixtures/mazerunner/cxx-scenarios-bugsnag/CMakeLists.txt index 8389ceb556..577809f3fd 100644 --- a/features/fixtures/mazerunner/cxx-scenarios-bugsnag/CMakeLists.txt +++ b/features/fixtures/mazerunner/cxx-scenarios-bugsnag/CMakeLists.txt @@ -1,7 +1,8 @@ cmake_minimum_required(VERSION 3.4.1) add_library(cxx-scenarios-bugsnag SHARED - src/main/cpp/cxx-scenarios-bugsnag.cpp) + src/main/cpp/cxx-scenarios-bugsnag.cpp + src/main/cpp/CXXExceptionSmokeScenario.cpp) add_library(lib_bugsnag SHARED IMPORTED) set(BUGSNAG_LIB_DIR diff --git a/features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/CXXExceptionSmokeScenario.cpp b/features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/CXXExceptionSmokeScenario.cpp new file mode 100644 index 0000000000..977ccc90d7 --- /dev/null +++ b/features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/CXXExceptionSmokeScenario.cpp @@ -0,0 +1,29 @@ +#include +#include + +namespace magicstacks { + +class FatalProblem : std::runtime_error { + using std::runtime_error::runtime_error; + + virtual ~FatalProblem() {} +}; + +void __attribute__((optnone)) top() { + throw new FatalProblem("well there it is!"); +} + +void __attribute__((optnone)) middle() { top(); } + +void __attribute__((optnone)) start() { middle(); } +} // namespace magicstacks + +extern "C" { +JNIEXPORT int JNICALL +Java_com_bugsnag_android_mazerunner_scenarios_CXXExceptionSmokeScenario_crash( + JNIEnv *env, jobject instance) { + magicstacks::start(); + + return 90; +} +} diff --git a/features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/cxx-scenarios-bugsnag.cpp b/features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/cxx-scenarios-bugsnag.cpp index 6c1ad586a4..72fc6b6581 100644 --- a/features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/cxx-scenarios-bugsnag.cpp +++ b/features/fixtures/mazerunner/cxx-scenarios-bugsnag/src/main/cpp/cxx-scenarios-bugsnag.cpp @@ -153,16 +153,6 @@ Java_com_bugsnag_android_mazerunner_scenarios_CXXExceptionOnErrorFalseScenario_c return 22; } -JNIEXPORT int JNICALL -Java_com_bugsnag_android_mazerunner_scenarios_CXXExceptionSmokeScenario_crash( - JNIEnv *env, - jobject instance) { - int x = 61; - printf("This one here: %ld\n", (long) f_trigger_an_exception(x > 0)); - printf("This one here: %ld\n", (long) f_throw_an_object(x > 0, x)); - return 55; -} - JNIEXPORT void JNICALL Java_com_bugsnag_android_mazerunner_scenarios_CXXStartScenario_activate( JNIEnv *env, diff --git a/features/smoke_tests/unhandled.feature b/features/smoke_tests/unhandled.feature index 5a7bc701d6..7d19e61a23 100644 --- a/features/smoke_tests/unhandled.feature +++ b/features/smoke_tests/unhandled.feature @@ -190,7 +190,7 @@ Scenario: C++ exception thrown with overwritten config # Exception details And the error payload field "events" is an array with 1 elements - And the exception "errorClass" demangles to "std::runtime_error*" + And the exception "errorClass" demangles to "magicstacks::FatalProblem*" And the exception "type" equals "c" And the event "unhandled" is true And the event "severity" equals "error" @@ -202,12 +202,11 @@ Scenario: C++ exception thrown with overwritten config # Stacktrace validation And the error payload field "events.0.exceptions.0.stacktrace" is a non-empty array And the event stacktrace identifies the program counter - And the event "exceptions.0.stacktrace.0.method" is not null - And the event "exceptions.0.stacktrace.0.file" is not null - And the error payload field "events.0.exceptions.0.stacktrace.0.frameAddress" is greater than 0 - And the error payload field "events.0.exceptions.0.stacktrace.0.symbolAddress" is greater than 0 - And the error payload field "events.0.exceptions.0.stacktrace.0.loadAddress" is greater than 0 - And the error payload field "events.0.exceptions.0.stacktrace.0.lineNumber" is greater than 0 + And the first significant stack frame methods and files should match: + | magicstacks::top() | | libcxx-scenarios-bugsnag.so | + | magicstacks::middle() | | libcxx-scenarios-bugsnag.so | + | magicstacks::start() | | libcxx-scenarios-bugsnag.so | + | Java_com_bugsnag_android_mazerunner_scenarios_CXXExceptionSmokeScenario_crash | | libcxx-scenarios-bugsnag.so | # App data And the event binary arch field is valid diff --git a/features/steps/android_steps.rb b/features/steps/android_steps.rb index a76c97201a..238afd31c2 100644 --- a/features/steps/android_steps.rb +++ b/features/steps/android_steps.rb @@ -117,7 +117,8 @@ method = frame["method"] if method == "_#{frame["method"]}" insignificant = method.start_with?("bsg_") || method.start_with?("std::") || - method.start_with?("__cxx") || + method.start_with?("__cx") || + method.start_with?("0x") || frame["file"].start_with?("/system/") || frame["file"].end_with?("libbugsnag-ndk.so") { :index => index, :method => method, :file => frame["file"] } unless insignificant