Use libunwind instead of libbacktrace to build stack traces. This fixes a segfault issue with platforms using libexecinfo and is generally more portable.

2019-02-27 23:34:49 -08:00 · 2019-02-27 23:34:49 -08:00 · 807f0dc01b
commit 807f0dc01b
parent 96ae461d34
11 changed files with 254 additions and 62 deletions
--- 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
--- 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)
--- 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
--- 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
--- a/cmake-utils/FindLibunwind.cmake
+++ 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, <adymo@kdevelop.org>
+#
+# 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 )
--- 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)
--- 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 <execinfo.h>
 #include <csignal>
-#include <iostream>
-#include <unistd.h>
 #include <cxxabi.h>
-#include <string>
 #include <sstream>
-#include <string>
-#include <dlfcn.h>
+
 #include "../logging/logging.h"
 #include <cpp-utils/process/SignalHandler.h>

+#define UNW_LOCAL_ONLY
+#include <libunwind.h>
+
 // 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 {
--- 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
--- 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<bool> 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
--- 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<SignalHandlerFunction* handler>
 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)
--- 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) {