From 807f0dc01b5f82c744b10a8adad80339fe2309f3 Mon Sep 17 00:00:00 2001 From: Sebastian Messmer Date: Wed, 27 Feb 2019 23:34:49 -0800 Subject: [PATCH] Use libunwind instead of libbacktrace to build stack traces. This fixes a segfault issue with platforms using libexecinfo and is generally more portable. --- .circleci/config.yml | 50 ++++++++++ CMakeLists.txt | 3 +- ChangeLog.txt | 1 + README.md | 5 +- cmake-utils/FindLibunwind.cmake | 44 +++++++++ src/cpp-utils/CMakeLists.txt | 15 +-- src/cpp-utils/assert/backtrace_nonwindows.cpp | 93 +++++++++++-------- src/cpp-utils/process/SignalCatcher.cpp | 3 +- src/cpp-utils/process/SignalHandler.cpp | 65 +++++++++++++ src/cpp-utils/process/SignalHandler.h | 6 ++ test/cpp-utils/assert/backtrace_test.cpp | 31 +++++-- 11 files changed, 254 insertions(+), 62 deletions(-) create mode 100644 cmake-utils/FindLibunwind.cmake diff --git a/.circleci/config.yml b/.circleci/config.yml index 6a5663e7..7b90b925 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -86,6 +86,50 @@ 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: @@ -189,6 +233,9 @@ 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 @@ -489,6 +536,9 @@ 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/CMakeLists.txt b/CMakeLists.txt index 45682b44..00018bfb 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -7,7 +7,8 @@ cmake_policy(SET CMP0054 NEW) project(cryfs) -include(cmake-utils/utils.cmake) +list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_LIST_DIR}/cmake-utils) +include(utils) require_gcc_version(5.0) require_clang_version(4.0) diff --git a/ChangeLog.txt b/ChangeLog.txt index cc2cf9cb..e1f9d1e8 100644 --- a/ChangeLog.txt +++ b/ChangeLog.txt @@ -7,6 +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. Other: * Updated to crypto++ 8.1 diff --git a/README.md b/README.md index 1309fc0d..41a99aef 100644 --- a/README.md +++ b/README.md @@ -56,14 +56,15 @@ 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-get 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 + $ 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 # Fedora - sudo dnf install git gcc-c++ cmake make libcurl-devel boost-devel boost-static openssl-devel fuse-devel python + sudo dnf install git gcc-c++ cmake make libcurl-devel boost-devel boost-static openssl-devel fuse-devel python libunwind-devel # Macintosh brew install cmake boost openssl libomp diff --git a/cmake-utils/FindLibunwind.cmake b/cmake-utils/FindLibunwind.cmake new file mode 100644 index 00000000..a41f7dd6 --- /dev/null +++ b/cmake-utils/FindLibunwind.cmake @@ -0,0 +1,44 @@ +# Taken from https://github.com/monero-project/monero/blob/31bdf7bd113c2576fe579ef3a25a2d8fef419ffc/cmake/FindLibunwind.cmake +# modifications: +# - remove linkage against gcc_eh because it was causing segfaults in various of our unit tests + +# - Try to find libunwind +# Once done this will define +# +# LIBUNWIND_FOUND - system has libunwind +# LIBUNWIND_INCLUDE_DIR - the libunwind include directory +# LIBUNWIND_LIBRARIES - Link these to use libunwind +# LIBUNWIND_DEFINITIONS - Compiler switches required for using libunwind + +# Copyright (c) 2006, Alexander Dymo, +# +# Redistribution and use is allowed according to the terms of the BSD license. +# For details see the accompanying COPYING-CMAKE-SCRIPTS file. + +find_path(LIBUNWIND_INCLUDE_DIR libunwind.h + /usr/include + /usr/local/include + ) + +find_library(LIBUNWIND_LIBRARIES NAMES unwind ) +if(NOT LIBUNWIND_LIBRARIES STREQUAL "LIBUNWIND_LIBRARIES-NOTFOUND") + if (CMAKE_COMPILER_IS_GNUCC) + set(LIBUNWIND_LIBRARIES "${LIBUNWIND_LIBRARIES}") + endif() +endif() + +# some versions of libunwind need liblzma, and we don't use pkg-config +# so we just look whether liblzma is installed, and add it if it is. +# It might not be actually needed, but doesn't hurt if it is not. +# We don't need any headers, just the lib, as it's privately needed. +message(STATUS "looking for liblzma") +find_library(LIBLZMA_LIBRARIES lzma ) +if(NOT LIBLZMA_LIBRARIES STREQUAL "LIBLZMA_LIBRARIES-NOTFOUND") + message(STATUS "liblzma found") + set(LIBUNWIND_LIBRARIES "${LIBUNWIND_LIBRARIES};${LIBLZMA_LIBRARIES}") +endif() + +include(FindPackageHandleStandardArgs) +find_package_handle_standard_args(Libunwind "Could not find libunwind" LIBUNWIND_INCLUDE_DIR LIBUNWIND_LIBRARIES) +# show the LIBUNWIND_INCLUDE_DIR and LIBUNWIND_LIBRARIES variables only in the advanced view +mark_as_advanced(LIBUNWIND_INCLUDE_DIR LIBUNWIND_LIBRARIES ) \ No newline at end of file diff --git a/src/cpp-utils/CMakeLists.txt b/src/cpp-utils/CMakeLists.txt index 2ecd690c..e1303bd8 100644 --- a/src/cpp-utils/CMakeLists.txt +++ b/src/cpp-utils/CMakeLists.txt @@ -61,13 +61,14 @@ set(SOURCES add_library(${PROJECT_NAME} STATIC ${SOURCES}) - -if(NOT MSVC) - find_package(Backtrace REQUIRED) - target_include_directories(${PROJECT_NAME} PUBLIC ${Backtrace_INCLUDE_DIRS}) - target_link_libraries(${PROJECT_NAME} PUBLIC ${Backtrace_LIBRARIES}) -else() - target_link_libraries(${PROJECT_NAME} PUBLIC DbgHelp) +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}) endif() if (NOT MSVC) diff --git a/src/cpp-utils/assert/backtrace_nonwindows.cpp b/src/cpp-utils/assert/backtrace_nonwindows.cpp index c495d0b6..5411bd55 100644 --- a/src/cpp-utils/assert/backtrace_nonwindows.cpp +++ b/src/cpp-utils/assert/backtrace_nonwindows.cpp @@ -1,18 +1,15 @@ #if !defined(_MSC_VER) -#include "backtrace.h" -#include #include -#include -#include #include -#include #include -#include -#include + #include "../logging/logging.h" #include +#define UNW_LOCAL_ONLY +#include + // TODO Add file and line number on non-windows using std::string; @@ -30,7 +27,12 @@ namespace { 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); @@ -41,46 +43,55 @@ namespace { } } - void pretty_print(std::ostream& str, const void *addr) { - Dl_info info; - if (0 == dladdr(addr, &info)) { - str << "[failed parsing line]"; - } else { - if (nullptr == info.dli_fname) { - str << "[no dli_fname]"; - } else { - str << info.dli_fname; - } - str << ":" << std::hex << info.dli_fbase << " "; - if (nullptr == info.dli_sname) { - str << "[no symbol name]"; - } else if (info.dli_sname[0] == '_') { - // is a mangled name - str << demangle(info.dli_sname); - } else { - // is not a mangled name - str << info.dli_sname; - } - str << " : " << std::hex << info.dli_saddr; - } - } + 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; - string backtrace_to_string(void *array[], size_t size) { - ostringstream result; - for (size_t i = 0; i < size; ++i) { - result << "#" << std::dec << i << " "; - pretty_print(result, array[i]); - result << "\n"; + 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 << ": "; } - return result.str(); + + 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() { - constexpr unsigned int MAX_SIZE = 100; - void *array[MAX_SIZE]; - size_t size = ::backtrace(array, MAX_SIZE); - return backtrace_to_string(array, size); + 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 { diff --git a/src/cpp-utils/process/SignalCatcher.cpp b/src/cpp-utils/process/SignalCatcher.cpp index c3fccb30..ea718a5d 100644 --- a/src/cpp-utils/process/SignalCatcher.cpp +++ b/src/cpp-utils/process/SignalCatcher.cpp @@ -88,7 +88,8 @@ public: , _registerer(signal, this) , _handler(signal) { // note: the order of the members ensures that: - // - when registering the signal handler fails, the registerer will be destroyed, unregistering the signal_occurred_flag, + // - when registering the signal handler, the SignalCatcher impl already has a valid _signal_occurred_flag set. + // - when registering the signal handler fails, the _registerer will be destroyed again, unregistering this SignalCatcherImpl, // i.e. there is no leak. // Allow only the set of signals that is supported on all platforms, see for Windows: https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/signal?view=vs-2017 diff --git a/src/cpp-utils/process/SignalHandler.cpp b/src/cpp-utils/process/SignalHandler.cpp index 26eafe4d..cb04186f 100644 --- a/src/cpp-utils/process/SignalHandler.cpp +++ b/src/cpp-utils/process/SignalHandler.cpp @@ -1 +1,66 @@ #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 ed8752d9..4af13568 100644 --- a/src/cpp-utils/process/SignalHandler.h +++ b/src/cpp-utils/process/SignalHandler.h @@ -20,11 +20,17 @@ 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 77d18bc1..708edaa7 100644 --- a/test/cpp-utils/assert/backtrace_test.cpp +++ b/test/cpp-utils/assert/backtrace_test.cpp @@ -31,11 +31,6 @@ TEST(BacktraceTest, ContainsBacktrace) { } #if !(defined(_MSC_VER) && defined(NDEBUG)) -TEST(BacktraceTest, ContainsExecutableName) { - string backtrace = cpputils::backtrace(); - EXPECT_THAT(backtrace, HasSubstr("cpp-utils-test")); -} - TEST(BacktraceTest, ContainsTopLevelLine) { string backtrace = cpputils::backtrace(); EXPECT_THAT(backtrace, HasSubstr("BacktraceTest")); @@ -92,22 +87,38 @@ TEST(BacktraceTest, DoesntCrashOnCaughtException) { #if !(defined(_MSC_VER) && defined(NDEBUG)) TEST(BacktraceTest, ShowBacktraceOnNullptrAccess) { auto output = call_process_exiting_with_nullptr_violation(); - EXPECT_THAT(output, HasSubstr("cpp-utils-test_exit_signal")); +#if !defined(_MSC_VER) + EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sig")); +#else + EXPECT_THAT(output, HasSubstr("handle_exit_signal")); +#endif } TEST(BacktraceTest, ShowBacktraceOnSigSegv) { auto output = call_process_exiting_with_sigsegv(); - EXPECT_THAT(output, HasSubstr("cpp-utils-test_exit_signal")); +#if defined(_MSC_VER) + EXPECT_THAT(output, HasSubstr("handle_exit_signal")); +#else + EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigsegv_handler(int)")); +#endif } TEST(BacktraceTest, ShowBacktraceOnUnhandledException) { auto output = call_process_exiting_with_exception("my_exception_message"); - EXPECT_THAT(output, HasSubstr("cpp-utils-test_exit_signal")); +#if defined(_MSC_VER) + EXPECT_THAT(output, HasSubstr("handle_exit_signal")); +#else + EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigabrt_handler(int)")); +#endif } TEST(BacktraceTest, ShowBacktraceOnSigIll) { auto output = call_process_exiting_with_sigill(); - EXPECT_THAT(output, HasSubstr("cpp-utils-test_exit_signal")); +#if defined(_MSC_VER) + EXPECT_THAT(output, HasSubstr("handle_exit_signal")); +#else + EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigill_handler(int)")); +#endif } #else TEST(BacktraceTest, ShowBacktraceOnNullptrAccess) { @@ -134,7 +145,7 @@ TEST(BacktraceTest, ShowBacktraceOnSigIll) { #if !defined(_MSC_VER) TEST(BacktraceTest, ShowBacktraceOnSigAbrt) { auto output = call_process_exiting_with_sigabrt(); - EXPECT_THAT(output, HasSubstr("cpp-utils-test_exit_signal")); + EXPECT_THAT(output, HasSubstr("cpputils::(anonymous namespace)::sigabrt_handler(int)")); } TEST(BacktraceTest, ShowBacktraceOnSigAbrt_ShowsCorrectSignalName) {