From 276e7f08e4bbfb3544d71b6f1f7adbe1ca592465 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Mon, 1 Apr 2019 19:18:49 -0700 Subject: [PATCH] Switch from libunwind to boost::stacktrace --- .circleci/config.yml | 68 ++-------- ChangeLog.txt | 2 +- README.md | 7 +- cmake-utils/utils.cmake | 4 +- src/cpp-utils/CMakeLists.txt | 17 ++- src/cpp-utils/assert/backtrace_nonwindows.cpp | 123 ++++-------------- src/cpp-utils/process/SignalHandler.cpp | 65 --------- src/cpp-utils/process/SignalHandler.h | 6 - test/cpp-utils/assert/backtrace_test.cpp | 35 +++-- 9 files changed, 73 insertions(+), 254 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index f92a541c..b9e70445 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -86,60 +86,16 @@ references: cmake --version /usr/local/bin/$CC --version /usr/local/bin/$CXX --version - upgrade_libunwind_pre: &upgrade_libunwind_pre - restore_cache: - keys: - # Find the most recent cache from any branch - - v1_upgrade_libunwind_cache_{{ checksum "/tmp/_build_env_vars" }}_{{ arch }} - upgrade_libunwind_post: &upgrade_libunwind_post - save_cache: - key: v1_upgrade_libunwind_cache_{{ checksum "/tmp/_build_env_vars" }}_{{ arch }} - paths: - - /tmp/libunwind-1.2.1 - upgrade_libunwind: &upgrade_libunwind - run: - name: Upgrade Libunwind - command: | - # We need to install libunwind manually because Circle CI uses Ubuntu 14.04 and the default libunwind version - # on that Ubuntu suffers from http://savannah.nongnu.org/bugs/?43752. - # This isn't an issue for any later version of Ubuntu. - - # Detect number of CPU cores - export NUMCORES=`nproc` - echo Using $NUMCORES cores - # Download and prepare libunwind (only if not already present from cache) - if [ ! -d "/tmp/libunwind-1.2.1" ]; then - echo "Didn't find libunwind in cache. Downloading and building." - wget -O /tmp/libunwind-1.2.1.tar.gz http://download.savannah.nongnu.org/releases/libunwind/libunwind-1.2.1.tar.gz - if [ $(sha512sum /tmp/libunwind-1.2.1.tar.gz | awk '{print $1;}') == "af7c280d2a963779a4a2711887618bc96383011e4e5d52e4085aa7fb351e55e357468f6ff85e66a216f1c6826538f498335a917a5970575c93be74c96316319b" ]; then - echo Correct sha512sum - else - echo Wrong sha512sum - sha512sum /tmp/libunwind-1.2.1.tar.gz - exit 1 - fi - echo Extracting... - tar -xf /tmp/libunwind-1.2.1.tar.gz -C /tmp - rm -rf /tmp/libunwind-1.2.1.tar.gz - cd /tmp/libunwind-1.2.1 - ./configure - make -j${NUMCORES} - else - echo Found libunwind in cache. Use cache and build. - fi - # Compile and install libunwind (if cached, this should be fast) - cd /tmp/libunwind-1.2.1 - sudo make -j${NUMCORES} install upgrade_boost_pre: &upgrade_boost_pre restore_cache: keys: # Find the most recent cache from any branch - - v3_upgrade_boost_cache_{{ checksum "/tmp/_build_env_vars" }}_{{ arch }} + - v4_upgrade_boost_cache_{{ checksum "/tmp/_build_env_vars" }}_{{ arch }} upgrade_boost_post: &upgrade_boost_post save_cache: - key: v3_upgrade_boost_cache_{{ checksum "/tmp/_build_env_vars" }}_{{ arch }} + key: v4_upgrade_boost_cache_{{ checksum "/tmp/_build_env_vars" }}_{{ arch }} paths: - - /tmp/boost_1_57_0 + - /tmp/boost_1_65_1 upgrade_boost: &upgrade_boost run: name: Upgrade Boost @@ -148,10 +104,10 @@ references: export NUMCORES=`nproc` echo Using $NUMCORES cores # Download and prepare boost (only if not already present from cache) - if [ ! -d "/tmp/boost_1_57_0" ]; then + if [ ! -d "/tmp/boost_1_65_1" ]; then echo "Didn't find boost in cache. Downloading and building." - wget -O /tmp/boost.tar.bz2 https://sourceforge.net/projects/boost/files/boost/1.57.0/boost_1_57_0.tar.bz2/download - if [ $(sha512sum /tmp/boost.tar.bz2 | awk '{print $1;}') == "61881440fd89644c43c6e3bc6292e9fed75a6d3a76f98654b189d0ed4e1087d77b585884e882270c08bf9f7132b173bfc1fde05848e06aa78ba7f1008d10714d" ]; then + wget -O /tmp/boost.tar.bz2 https://sourceforge.net/projects/boost/files/boost/1.65.1/boost_1_65_1.tar.bz2/download + if [ $(sha512sum /tmp/boost.tar.bz2 | awk '{print $1;}') == "a9e6866d3bb3e7c198f442ff09f5322f58064dca79bc420f2f0168eb63964226dfbc4f034a5a5e5958281fdf7518a1b057c894fbda0b61fced59c1661bf30f1a" ]; then echo Correct sha512sum else echo Wrong sha512sum @@ -161,14 +117,14 @@ references: echo Extracting... tar -xf /tmp/boost.tar.bz2 -C /tmp rm -rf boost.tar.bz2 - cd /tmp/boost_1_57_0 + cd /tmp/boost_1_65_1 ./bootstrap.sh --with-toolset=${BUILD_TOOLSET} --with-libraries=filesystem,thread,chrono,program_options cd .. else echo Found boost in cache. Use cache and build. fi # Compile and install boost (if cached, this should be fast) - cd /tmp/boost_1_57_0 + cd /tmp/boost_1_65_1 sudo ./b2 toolset=${BUILD_TOOLSET} link=static cxxflags=-fPIC -d0 -j$NUMCORES install build_pre: &build_pre restore_cache: @@ -233,9 +189,6 @@ references: - <<: *container_setup_pre - <<: *container_setup - <<: *container_setup_post - - <<: *upgrade_libunwind_pre - - <<: *upgrade_libunwind - - <<: *upgrade_libunwind_post - <<: *upgrade_boost_pre - <<: *upgrade_boost - <<: *upgrade_boost_post @@ -514,7 +467,7 @@ jobs: OMP_NUM_THREADS: "1" CXXFLAGS: "-O2 -fsanitize=thread -fno-omit-frame-pointer" BUILD_TYPE: "Debug" - GTEST_ARGS: "--gtest_filter=-LoggingTest.LoggingAlsoWorksAfterFork:AssertTest_DebugBuild.*:SignalCatcherTest.*_thenDies:SignalHandlerTest.*_thenDies:SignalHandlerTest.givenMultipleSigIntHandlers_whenRaising_thenCatchesCorrectSignal:CliTest_Setup.*:CliTest_IntegrityCheck.*:*/CliTest_WrongEnvironment.*:CliTest_Unmount.*" + GTEST_ARGS: "--gtest_filter=-LoggingTest.LoggingAlsoWorksAfterFork:AssertTest_*:BacktraceTest.*:SignalCatcherTest.*_thenDies:SignalHandlerTest.*_thenDies:SignalHandlerTest.givenMultipleSigIntHandlers_whenRaising_thenCatchesCorrectSignal:CliTest_Setup.*:CliTest_IntegrityCheck.*:*/CliTest_WrongEnvironment.*:CliTest_Unmount.*" CMAKE_FLAGS: "" RUN_TESTS: true clang_tidy: @@ -524,9 +477,6 @@ jobs: - <<: *container_setup_pre - <<: *container_setup - <<: *container_setup_post - - <<: *upgrade_libunwind_pre - - <<: *upgrade_libunwind - - <<: *upgrade_libunwind_post - <<: *upgrade_boost_pre - <<: *upgrade_boost - <<: *upgrade_boost_post diff --git a/ChangeLog.txt b/ChangeLog.txt index 90d2170d..3088ecbc 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -7,7 +7,7 @@ Fixed bugs: Compatibility: * Fixed some incompatibilities with systems using the musl libc -* Use libunwind instead of libbacktrace to build stack traces. This fixes a segfault issue with platforms using libexecinfo and is generally more portable. +* Use boost::stacktrace instead of libbacktrace to build stack traces. This fixes a segfault issue with platforms using libexecinfo and is generally more portable. Other: * Updated to crypto++ 8.1 diff --git a/README.md b/README.md index ffea44e6..b0f4a684 100644 --- a/README.md +++ b/README.md @@ -46,7 +46,7 @@ Requirements - GCC version >= 5.0 or Clang >= 4.0 - CMake version >= 3.0 - libcurl4 (including development headers) - - Boost libraries version >= 1.57 (including development headers) + - Boost libraries version >= 1.65.1 (including development headers) - filesystem - system - chrono @@ -56,15 +56,14 @@ Requirements - libFUSE version >= 2.8.6 (including development headers), on Mac OS X instead install osxfuse from https://osxfuse.github.io/ - Python >= 2.7 - OpenMP - - Libunwind You can use the following commands to install these requirements # Ubuntu - $ sudo apt install git g++ cmake make libcurl4-openssl-dev libboost-filesystem-dev libboost-system-dev libboost-chrono-dev libboost-program-options-dev libboost-thread-dev libssl-dev libfuse-dev python libunwind-dev + $ sudo apt install git g++ cmake make libcurl4-openssl-dev libboost-filesystem-dev libboost-system-dev libboost-chrono-dev libboost-program-options-dev libboost-thread-dev libssl-dev libfuse-dev python # Fedora - sudo dnf install git gcc-c++ cmake make libcurl-devel boost-devel boost-static openssl-devel fuse-devel python libunwind-devel + sudo dnf install git gcc-c++ cmake make libcurl-devel boost-devel boost-static openssl-devel fuse-devel python # Macintosh brew install cmake boost openssl libomp diff --git a/cmake-utils/utils.cmake b/cmake-utils/utils.cmake index 72cac168..da4dff8c 100644 --- a/cmake-utils/utils.cmake +++ b/cmake-utils/utils.cmake @@ -108,7 +108,7 @@ endfunction(target_enable_style_warnings) function(target_add_boost TARGET) # Load boost libraries if(NOT DEFINED Boost_USE_STATIC_LIBS OR Boost_USE_STATIC_LIBS) - # Many supported systems don't have boost >= 1.57. Better link it statically. + # Many supported systems don't have boost >= 1.65.1. Better link it statically. message(STATUS "Boost will be statically linked") set(Boost_USE_STATIC_LIBS ON) else(NOT DEFINED Boost_USE_STATIC_LIBS OR Boost_USE_STATIC_LIBS) @@ -116,7 +116,7 @@ function(target_add_boost TARGET) set(Boost_USE_STATIC_LIBS OFF) endif(NOT DEFINED Boost_USE_STATIC_LIBS OR Boost_USE_STATIC_LIBS) set(BOOST_THREAD_VERSION 4) - find_package(Boost 1.57.0 + find_package(Boost 1.65.1 REQUIRED COMPONENTS ${ARGN}) target_include_directories(${TARGET} SYSTEM PUBLIC ${Boost_INCLUDE_DIRS}) diff --git a/src/cpp-utils/CMakeLists.txt b/src/cpp-utils/CMakeLists.txt index e1303bd8..80688108 100644 --- a/src/cpp-utils/CMakeLists.txt +++ b/src/cpp-utils/CMakeLists.txt @@ -63,12 +63,17 @@ add_library(${PROJECT_NAME} STATIC ${SOURCES}) if(MSVC) target_link_libraries(${PROJECT_NAME} PUBLIC DbgHelp) -elseif(NOT APPLE) - # note: We use the libunwind code on Apple, but we don't seem to need these lines to link against it. - find_package(Libunwind REQUIRED) - target_include_directories(${PROJECT_NAME} PRIVATE ${LIBUNWIND_INCLUDE_DIR}) - target_link_libraries(${PROJECT_NAME} PRIVATE ${LIBUNWIND_LIBRARIES}) - target_compile_definitions(${PROJECT_NAME} PRIVATE ${LIBUNWIND_DEFINITIONS}) +elseif (APPLE) + target_compile_definitions(${PROJECT_NAME} PRIVATE BOOST_STACKTRACE_GNU_SOURCE_NOT_REQUIRED) +else() + find_program(ADDR2LINE addr2line) + if ("${ADDR2LINE}" STREQUAL "ADDR2LINE-NOTFOUND") + message(WARNING "addr2line not found. Backtraces will be reduced.") + else() + message(STATUS "addr2line found. Using it for backtraces.") + target_compile_definitions(${PROJECT_NAME} PRIVATE BOOST_STACKTRACE_USE_ADDR2LINE) + target_compile_definitions(${PROJECT_NAME} PRIVATE BOOST_STACKTRACE_ADDR2LINE_LOCATION=${ADDR2LINE}) + endif() endif() if (NOT MSVC) diff --git a/src/cpp-utils/assert/backtrace_nonwindows.cpp b/src/cpp-utils/assert/backtrace_nonwindows.cpp index 5411bd55..8fbcac60 100644 --- a/src/cpp-utils/assert/backtrace_nonwindows.cpp +++ b/src/cpp-utils/assert/backtrace_nonwindows.cpp @@ -1,16 +1,12 @@ #if !defined(_MSC_VER) #include -#include #include #include "../logging/logging.h" #include -#define UNW_LOCAL_ONLY -#include - -// TODO Add file and line number on non-windows +#include using std::string; using std::ostringstream; @@ -18,105 +14,36 @@ using namespace cpputils::logging; namespace cpputils { -namespace { - std::string demangle(const string &mangledName) { - string result; - int status = -10; - char *demangledName = nullptr; - try { - demangledName = abi::__cxa_demangle(mangledName.c_str(), NULL, NULL, &status); - if (status == 0) { - result = demangledName; - } else if (status == -2) { - // mangledName was not a c++ mangled name, probably because it's a C name like for static - // initialization or stuff. Let's just return the name instead. - result = mangledName; - } else { - // other error - result = "[demangling error " + std::to_string(status) + "]" + mangledName; - } - free(demangledName); - return result; - } catch (...) { - free(demangledName); - throw; - } - } - - void pretty_print(std::ostringstream& str, unw_cursor_t* cursor) { - constexpr unsigned int MAXNAMELEN=256; - char name[MAXNAMELEN]; - unw_word_t offp = 0, ip = 0; - - int status = unw_get_reg(cursor, UNW_REG_IP, &ip); - if (0 != status) { - str << "[unw_get_reg error: " << status << "]: "; - } else { - str << "0x" << std::hex << ip << ": "; - } - - status = unw_get_proc_name(cursor, name, MAXNAMELEN, &offp); - if (0 != status) { - str << "[unw_get_proc_name error: " << status << "]"; - } else { - str << demangle(name); - } - str << " +0x" << std::hex << offp; - } +string backtrace() { + std::ostringstream str; + str << boost::stacktrace::stacktrace(); + return str.str(); } - string backtrace() { - std::ostringstream result; - - unw_context_t uc; - int status = unw_getcontext(&uc); - if (0 != status) { - return "[unw_getcontext error: " + std::to_string(status) + "]"; - } - - unw_cursor_t cursor; - status = unw_init_local(&cursor, &uc); - if (0 != status) { - return "[unw_init_local error: " + std::to_string(status) + "]"; - } - - - size_t line = 0; - while ((status = unw_step(&cursor)) > 0) { - result << "#" << std::dec << (line++) << " "; - pretty_print(result, &cursor); - result << "\n"; - } - if (status != 0) { - result << "[unw_step error :" << status << "]"; - } - - return result.str(); - } - namespace { - void sigsegv_handler(int) { - LOG(ERR, "SIGSEGV\n{}", backtrace()); - exit(1); - } - void sigill_handler(int) { - LOG(ERR, "SIGILL\n{}", backtrace()); - exit(1); - } - void sigabrt_handler(int) { - LOG(ERR, "SIGABRT\n{}", backtrace()); - exit(1); - } +void sigsegv_handler(int) { + LOG(ERR, "SIGSEGV\n{}", backtrace()); + exit(1); +} +void sigill_handler(int) { + LOG(ERR, "SIGILL\n{}", backtrace()); + exit(1); +} +void sigabrt_handler(int) { + LOG(ERR, "SIGABRT\n{}", backtrace()); + exit(1); +} } - void showBacktraceOnCrash() { - // the signal handler RAII objects will be initialized on first call (which will register the signal handler) - // and destroyed on program exit (which will unregister the signal handler) +void showBacktraceOnCrash() { + // the signal handler RAII objects will be initialized on first call (which will register the signal handler) + // and destroyed on program exit (which will unregister the signal handler) + + static SignalHandlerRAII<&sigsegv_handler> segv(SIGSEGV); + static SignalHandlerRAII<&sigabrt_handler> abrt(SIGABRT); + static SignalHandlerRAII<&sigill_handler> ill(SIGILL); +} - static SignalHandlerRAII<&sigsegv_handler> segv(SIGSEGV); - static SignalHandlerRAII<&sigabrt_handler> abrt(SIGABRT); - static SignalHandlerRAII<&sigill_handler> ill(SIGILL); - } } #endif diff --git a/src/cpp-utils/process/SignalHandler.cpp b/src/cpp-utils/process/SignalHandler.cpp index cb04186f..26eafe4d 100644 --- a/src/cpp-utils/process/SignalHandler.cpp +++ b/src/cpp-utils/process/SignalHandler.cpp @@ -1,66 +1 @@ #include "SignalHandler.h" - -#if !defined(_MSC_VER) - -namespace cpputils { -namespace detail { -namespace { - -std::atomic already_checked_for_libunwind_bug(false); - -} - -sigset_t _sigemptyset() { - sigset_t result; - int error = sigemptyset(&result); - if (0 != error) { - throw std::runtime_error("Error calling sigemptyset. Errno: " + std::to_string(errno)); - } - return result; -} - -void _sigmask(sigset_t* new_value, sigset_t* old_value) { - int error = pthread_sigmask(SIG_SETMASK, new_value, old_value); - if (0 != error) { - throw std::runtime_error("Error calling pthread_sigmask. Errno: " + std::to_string(errno)); - } -} - -// Check that we're not running into http://savannah.nongnu.org/bugs/?43752 -void check_against_libunwind_bug() { - if (already_checked_for_libunwind_bug.exchange(true)) { - return; - } - - // set new signal handler - sigset_t old_value = _sigemptyset(); - sigset_t new_value = _sigemptyset(); - - _sigmask(&new_value, &old_value); - - sigset_t before_exception = _sigemptyset(); - _sigmask(nullptr, &before_exception); - - // throw an exception - try { - throw std::runtime_error("Some exception"); - } catch (const std::exception &e) {} - - sigset_t after_exception = _sigemptyset(); - _sigmask(nullptr, &after_exception); - - // reset to old signal handler - _sigmask(&old_value, nullptr); - - // check that the exception didn't screw up the signal mask - if (0 != std::memcmp(&before_exception, &after_exception, sizeof(sigset_t))) { // NOLINT(cppcoreguidelines-pro-type-union-access) - ASSERT(false, - "Throwing an exception screwed up the signal mask. You likely ran into this bug: http://savannah.nongnu.org/bugs/?43752 . Please build CryFS against a newer version of libunwind or build libunwind with --disable-cxx-exceptions."); - } -} - - -} -} - -#endif diff --git a/src/cpp-utils/process/SignalHandler.h b/src/cpp-utils/process/SignalHandler.h index 4af13568..ed8752d9 100644 --- a/src/cpp-utils/process/SignalHandler.h +++ b/src/cpp-utils/process/SignalHandler.h @@ -20,17 +20,11 @@ using SignalHandlerFunction = void(int); #if !defined(_MSC_VER) -namespace detail { -void check_against_libunwind_bug(); -} - template class SignalHandlerRAII final { public: explicit SignalHandlerRAII(int signal) : _old_handler(), _signal(signal) { - detail::check_against_libunwind_bug(); - struct sigaction new_signal_handler{}; std::memset(&new_signal_handler, 0, sizeof(new_signal_handler)); new_signal_handler.sa_handler = handler; // NOLINT(cppcoreguidelines-pro-type-union-access) diff --git a/test/cpp-utils/assert/backtrace_test.cpp b/test/cpp-utils/assert/backtrace_test.cpp index 708edaa7..a5b46709 100644 --- a/test/cpp-utils/assert/backtrace_test.cpp +++ b/test/cpp-utils/assert/backtrace_test.cpp @@ -25,12 +25,8 @@ namespace { } } -TEST(BacktraceTest, ContainsBacktrace) { - string backtrace = cpputils::backtrace(); - EXPECT_THAT(backtrace, HasSubstr("#1")); -} - #if !(defined(_MSC_VER) && defined(NDEBUG)) + TEST(BacktraceTest, ContainsTopLevelLine) { string backtrace = cpputils::backtrace(); EXPECT_THAT(backtrace, HasSubstr("BacktraceTest")); @@ -85,12 +81,21 @@ TEST(BacktraceTest, DoesntCrashOnCaughtException) { } #if !(defined(_MSC_VER) && defined(NDEBUG)) +TEST(BacktraceTest, ContainsBacktrace) { + string backtrace = cpputils::backtrace(); +#if defined(_MSC_VER) + EXPECT_THAT(backtrace, HasSubstr("testing::Test::Run")); +#else + EXPECT_THAT(backtrace, HasSubstr("BacktraceTest_ContainsBacktrace_Test::TestBody")); +#endif +} + TEST(BacktraceTest, ShowBacktraceOnNullptrAccess) { auto output = call_process_exiting_with_nullptr_violation(); -#if !defined(_MSC_VER) - EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sig")); -#else +#if defined(_MSC_VER) EXPECT_THAT(output, HasSubstr("handle_exit_signal")); +#else + EXPECT_THAT(output, HasSubstr("cpputils::backtrace")); #endif } @@ -99,7 +104,7 @@ TEST(BacktraceTest, ShowBacktraceOnSigSegv) { #if defined(_MSC_VER) EXPECT_THAT(output, HasSubstr("handle_exit_signal")); #else - EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigsegv_handler(int)")); + EXPECT_THAT(output, HasSubstr("cpputils::backtrace")); #endif } @@ -108,19 +113,23 @@ TEST(BacktraceTest, ShowBacktraceOnUnhandledException) { #if defined(_MSC_VER) EXPECT_THAT(output, HasSubstr("handle_exit_signal")); #else - EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigabrt_handler(int)")); + EXPECT_THAT(output, HasSubstr("cpputils::backtrace")); #endif } TEST(BacktraceTest, ShowBacktraceOnSigIll) { auto output = call_process_exiting_with_sigill(); #if defined(_MSC_VER) - EXPECT_THAT(output, HasSubstr("handle_exit_signal")); + EXPECT_THAT(output, HasSubstr("handle_exit_signal")); #else - EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigill_handler(int)")); + EXPECT_THAT(output, HasSubstr("cpputils::backtrace")); #endif } #else +TEST(BacktraceTest, ContainsBacktrace) { + string backtrace = cpputils::backtrace(); + EXPECT_THAT(backtrace, HasSubstr("#1")); +} TEST(BacktraceTest, ShowBacktraceOnNullptrAccess) { auto output = call_process_exiting_with_nullptr_violation(); EXPECT_THAT(output, HasSubstr("#1")); @@ -145,7 +154,7 @@ TEST(BacktraceTest, ShowBacktraceOnSigIll) { #if !defined(_MSC_VER) TEST(BacktraceTest, ShowBacktraceOnSigAbrt) { auto output = call_process_exiting_with_sigabrt(); - EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigabrt_handler(int)")); + EXPECT_THAT(output, HasSubstr("cpputils::backtrace")); } TEST(BacktraceTest, ShowBacktraceOnSigAbrt_ShowsCorrectSignalName) {