Merge pull request #1022 from BehaviorTree/thread_sanitizer

Thread and Address sanitizers (#1019 + fixes)

Author: facontidavide

.github/workflows/cmake_ubuntu_sanitizers.yml

@@ -0,0 +1,69 @@
+name: cmake Ubuntu Sanitizers
+
+on:
+  push:
+    branches:
+      - master
+  pull_request:
+    types: [opened, synchronize, reopened]
+
+env:
+  # Customize the CMake build type here (Release, Debug, RelWithDebInfo, etc.)
+  BUILD_TYPE: Debug
+
+jobs:
+  build:
+    # The CMake configure and build commands are platform agnostic and should work equally
+    # well on Windows or Mac.  You can convert this to a matrix build if you need
+    # cross-platform coverage.
+    # See: https://docs.github.com/en/free-pro-team@latest/actions/learn-github-actions/managing-complex-workflows#using-a-build-matrix
+    runs-on: ${{ matrix.os }}
+    strategy:
+      fail-fast: false
+      matrix:
+        os: [ubuntu-22.04]
+        sanitizer: [asan_ubsan, tsan]
+
+    steps:
+    - uses: actions/checkout@v2
+
+    - name: Install Conan
+      id: conan
+      uses: turtlebrowser/get-conan@main
+
+    - name: Create default profile
+      run: conan profile detect
+
+    - name: Install conan dependencies
+      run: conan install conanfile.py -s build_type=${{env.BUILD_TYPE}} --build=missing
+
+    - name: Normalize build type
+      shell: bash
+      # The build type is Capitalized, e.g. Release, but the preset is all lowercase, e.g. release.
+      # There is no built in way to do string manipulations on GHA as far as I know.`
+      run: echo "BUILD_TYPE_LOWERCASE=$(echo "${BUILD_TYPE}" | tr '[:upper:]' '[:lower:]')" >> $GITHUB_ENV
+
+    - name: Configure CMake
+      shell: bash
+      run: |
+        if [[ "${{ matrix.sanitizer }}" == "asan_ubsan" ]]; then
+          cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} \
+            -DBTCPP_ENABLE_ASAN:BOOL=ON -DBTCPP_ENABLE_UBSAN:BOOL=ON
+        else
+          cmake --preset conan-${{ env.BUILD_TYPE_LOWERCASE }} \
+            -DBTCPP_ENABLE_TSAN:BOOL=ON
+        fi
+
+    - name: Build
+      shell: bash
+      run: cmake --build --preset conan-${{ env.BUILD_TYPE_LOWERCASE }}
+
+    - name: run test (Linux + Address and Undefined Behavior Sanitizers)
+      env:
+        GTEST_COLOR: "On"
+        ASAN_OPTIONS: "color=always"
+        UBSAN_OPTIONS: "halt_on_error=1:print_stacktrace=1:color=always"
+        TSAN_OPTIONS: "color=always"
+      # There is a known issue with TSAN on recent kernel versions. Without the vm.mmap_rnd_bits=28
+      # workaround all binaries with TSan enabled crash with "FATAL: ThreadSanitizer: unexpected memory mapping"
+      run: sudo sysctl vm.mmap_rnd_bits=28 && ctest --test-dir build/${{env.BUILD_TYPE}} --output-on-failure

CMakeLists.txt

@@ -13,6 +13,9 @@ option(BTCPP_EXAMPLES   "Build tutorials and examples" ON)
 option(BUILD_TESTING "Build the unit tests" ON)
 option(BTCPP_GROOT_INTERFACE "Add Groot2 connection. Requires ZeroMQ" ON)
 option(BTCPP_SQLITE_LOGGING "Add SQLite logging." ON)
+option(BTCPP_ENABLE_ASAN "Enable Address Sanitizer" OFF)
+option(BTCPP_ENABLE_UBSAN "Enable Undefined Behavior Sanitizer" OFF)
+option(BTCPP_ENABLE_TSAN "Enable Thread Sanitizer" OFF)
 
 option(USE_V3_COMPATIBLE_NAMES  "Use some alias to compile more easily old 3.x code" OFF)
 option(ENABLE_FUZZING "Enable fuzzing builds" OFF)
@@ -54,6 +57,8 @@ endif()
 set(CMAKE_CONFIG_PATH ${CMAKE_MODULE_PATH} "${CMAKE_CURRENT_LIST_DIR}/cmake")
 list(APPEND CMAKE_MODULE_PATH "${CMAKE_CONFIG_PATH}")
 
+include(sanitizers)
+
 set(BTCPP_LIBRARY ${PROJECT_NAME})
 
 if(NOT CMAKE_BUILD_TYPE AND NOT CMAKE_CONFIGURATION_TYPES)

cmake/sanitizers.cmake

@@ -0,0 +1,31 @@
+if(BTCPP_ENABLE_ASAN OR BTCPP_ENABLE_UBSAN OR BTCPP_ENABLE_TSAN)
+    if(NOT CMAKE_BUILD_TYPE MATCHES "Debug|RelWithDebInfo")
+        message(FATAL_ERROR "Sanitizers require debug symbols. Please set CMAKE_BUILD_TYPE to Debug or RelWithDebInfo.")
+    endif()
+    add_compile_options(-fno-omit-frame-pointer)
+endif()
+
+# Address Sanitizer and Undefined Behavior Sanitizer can be run at the same time.
+# Thread Sanitizer requires its own build.
+if(BTCPP_ENABLE_TSAN AND (BTCPP_ENABLE_ASAN OR BTCPP_ENABLE_UBSAN))
+    message(FATAL_ERROR "TSan is not compatible with ASan or UBSan. ASan and UBSan can run together, but TSan requires its own separate build.")
+endif()
+
+if(BTCPP_ENABLE_ASAN)
+    message(STATUS "Address Sanitizer enabled")
+    add_compile_options(-fsanitize=address)
+    add_link_options(-fsanitize=address)
+    endif()
+
+    if(BTCPP_ENABLE_UBSAN)
+    message(STATUS "Undefined Behavior Sanitizer enabled")
+    add_compile_options(-fsanitize=undefined)
+    add_link_options(-fsanitize=undefined)
+    endif()
+
+    if(BTCPP_ENABLE_TSAN)
+    message(STATUS "Thread Sanitizer enabled")
+    add_compile_options(-fsanitize=thread)
+    add_link_options(-fsanitize=thread)
+    add_compile_definitions(USE_SANITIZE_THREAD)
+endif()

include/behaviortree_cpp/blackboard.h

@@ -141,7 +141,7 @@ class Blackboard
   const Blackboard* rootBlackboard() const;
 
 private:
-  mutable std::mutex mutex_;
+  mutable std::mutex storage_mutex_;
   mutable std::recursive_mutex entry_mutex_;
   std::unordered_map<std::string, std::shared_ptr<Entry>> storage_;
   std::weak_ptr<Blackboard> parent_bb_;
@@ -186,7 +186,7 @@ inline T Blackboard::get(const std::string& key) const
 
 inline void Blackboard::unset(const std::string& key)
 {
-  std::unique_lock lock(mutex_);
+  std::unique_lock storage_lock(storage_mutex_);
 
   // check local storage
   auto it = storage_.find(key);
@@ -207,15 +207,15 @@ inline void Blackboard::set(const std::string& key, const T& value)
     rootBlackboard()->set(key.substr(1, key.size() - 1), value);
     return;
   }
-  std::unique_lock lock(mutex_);
+  std::unique_lock storage_lock(storage_mutex_);
 
   // check local storage
   auto it = storage_.find(key);
   if(it == storage_.end())
   {
     // create a new entry
     Any new_value(value);
-    lock.unlock();
+    storage_lock.unlock();
     std::shared_ptr<Blackboard::Entry> entry;
     // if a new generic port is created with a string, it's type should be AnyTypeAllowed
     if constexpr(std::is_same_v<std::string, T>)
@@ -228,7 +228,7 @@ inline void Blackboard::set(const std::string& key, const T& value)
                         GetAnyFromStringFunctor<T>());
       entry = createEntryImpl(key, new_port);
     }
-    lock.lock();
+    storage_lock.lock();
 
     entry->value = new_value;
     entry->sequence_id++;
@@ -239,6 +239,8 @@ inline void Blackboard::set(const std::string& key, const T& value)
     // this is not the first time we set this entry, we need to check
     // if the type is the same or not.
     Entry& entry = *it->second;
+    storage_lock.unlock();
+
     std::scoped_lock scoped_lock(entry.entry_mutex);
 
     Any& previous_any = entry.value;

include/behaviortree_cpp/utils/timer_queue.h

@@ -23,10 +23,7 @@ class Semaphore
 
   void notify()
   {
-    {
-      std::lock_guard<std::mutex> lock(m_mtx);
-      m_count++;
-    }
+    m_count.fetch_add(1);
     m_cv.notify_one();
   }
 
@@ -38,8 +35,15 @@ class Semaphore
     {
       return false;
     }
-    m_count--;
+    // Only decrement if there is a real count. If we woke because of manualUnlock,
+    // m_count may be zero and we must not decrement it.
+    if(m_count > 0)
+    {
+      m_count.fetch_sub(1);
+    }
+    // Clear the manual unlock flag
     m_unlock = false;
+
     return true;
   }
 
@@ -52,7 +56,7 @@ class Semaphore
 private:
   std::mutex m_mtx;
   std::condition_variable m_cv;
-  unsigned m_count = 0;
+  std::atomic_uint m_count = 0;
   std::atomic_bool m_unlock = false;
 };
 }  // namespace details
@@ -74,15 +78,18 @@ class TimerQueue
 public:
   TimerQueue()
   {
-    m_th = std::thread([this] { run(); });
+    m_finish.store(false);
+    m_thread = std::thread([this]() { run(); });
   }
 
   ~TimerQueue()
   {
-    m_finish = true;
+    m_finish.store(true);
     cancelAll();
-    m_checkWork.manualUnlock();
-    m_th.join();
+    if(m_thread.joinable())
+    {
+      m_thread.join();
+    }
   }
 
   //! Adds a new timer
@@ -174,7 +181,7 @@ class TimerQueue
 
   void run()
   {
-    while(!m_finish)
+    while(!m_finish.load())
     {
       auto end = calcWaitTime();
       if(end.first)
@@ -239,8 +246,8 @@ class TimerQueue
   }
 
   details::Semaphore m_checkWork;
-  std::thread m_th;
-  bool m_finish = false;
+  std::thread m_thread;
+  std::atomic_bool m_finish = false;
   uint64_t m_idcounter = 0;
 
   struct WorkItem

src/blackboard.cpp

@@ -52,11 +52,13 @@ Blackboard::getEntry(const std::string& key) const
     return rootBlackboard()->getEntry(key.substr(1, key.size() - 1));
   }
 
-  std::unique_lock<std::mutex> lock(mutex_);
-  auto it = storage_.find(key);
-  if(it != storage_.end())
   {
-    return it->second;
+    std::unique_lock<std::mutex> storage_lock(storage_mutex_);
+    auto it = storage_.find(key);
+    if(it != storage_.end())
+    {
+      return it->second;
+    }
   }
   // not found. Try autoremapping
   if(auto parent = parent_bb_.lock())
@@ -130,7 +132,7 @@ std::vector<StringView> Blackboard::getKeys() const
 
 void Blackboard::clear()
 {
-  std::unique_lock<std::mutex> lock(mutex_);
+  std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   storage_.clear();
 }
 
@@ -157,8 +159,10 @@ void Blackboard::createEntry(const std::string& key, const TypeInfo& info)
 
 void Blackboard::cloneInto(Blackboard& dst) const
 {
-  std::unique_lock lk1(mutex_);
-  std::unique_lock lk2(dst.mutex_);
+  // Lock both mutexes without risking lock-order inversion.
+  std::unique_lock<std::mutex> lk1(storage_mutex_, std::defer_lock);
+  std::unique_lock<std::mutex> lk2(dst.storage_mutex_, std::defer_lock);
+  std::lock(lk1, lk2);
 
   // keys that are not updated must be removed.
   std::unordered_set<std::string> keys_to_remove;
@@ -212,7 +216,7 @@ Blackboard::Ptr Blackboard::parent()
 std::shared_ptr<Blackboard::Entry> Blackboard::createEntryImpl(const std::string& key,
                                                                const TypeInfo& info)
 {
-  std::unique_lock<std::mutex> lock(mutex_);
+  std::unique_lock<std::mutex> storage_lock(storage_mutex_);
   // This function might be called recursively, when we do remapping, because we move
   // to the top scope to find already existing  entries
 

src/tree_node.cpp

@@ -106,19 +106,13 @@ NodeStatus TreeNode::executeTick()
     if(!substituted)
     {
       using namespace std::chrono;
-
-      auto t1 = steady_clock::now();
-      // trick to prevent the compile from reordering the order of execution. See #861
-      // This makes sure that the code is executed at the end of this scope
-      std::shared_ptr<void> execute_later(nullptr, [&](...) {
-        auto t2 = steady_clock::now();
-        if(monitor_tick)
-        {
-          monitor_tick(*this, new_status, duration_cast<microseconds>(t2 - t1));
-        }
-      });
-
+      const auto t1 = steady_clock::now();
       new_status = tick();
+      const auto t2 = steady_clock::now();
+      if(monitor_tick)
+      {
+        monitor_tick(*this, new_status, duration_cast<microseconds>(t2 - t1));
+      }
     }
   }
 

tests/CMakeLists.txt

@@ -43,16 +43,28 @@ if(ament_cmake_FOUND)
 
 else()
 
+    enable_testing()
+
     find_package(GTest REQUIRED)
+    include(GoogleTest)
 
-    enable_testing()
     add_executable(behaviortree_cpp_test ${BT_TESTS})
-    add_test(NAME btcpp_test COMMAND behaviortree_cpp_test)
 
     target_link_libraries(behaviortree_cpp_test
         GTest::gtest
         GTest::gtest_main)
 
+    # gtest_discover_tests queries the test executable for available tests and registers them on ctest individually
+    # On Windows it needs a little help to find the shared libraries
+    if(WIN32)
+        gtest_discover_tests(behaviortree_cpp_test
+            DISCOVERY_MODE PRE_TEST
+            DISCOVERY_ENVIRONMENT "PATH=$<TARGET_FILE_DIR:behaviortree_cpp_test>;$ENV{PATH}"
+        )
+    else()
+        gtest_discover_tests(behaviortree_cpp_test)
+    endif()
+
 endif()
 
 target_include_directories(behaviortree_cpp_test PRIVATE include)

tests/gtest_blackboard.cpp

@@ -294,6 +294,8 @@ TEST(BlackboardTest, CheckTypeSafety)
   ASSERT_TRUE(is);
 }
 
+#ifndef USE_SANITIZE_THREAD
+
 TEST(BlackboardTest, AnyPtrLocked)
 {
   auto blackboard = Blackboard::create();
@@ -346,6 +348,7 @@ TEST(BlackboardTest, AnyPtrLocked)
     ASSERT_NE(cycles, value);
   }
 }
+#endif
 
 TEST(BlackboardTest, SetStringView)
 {

tests/gtest_decorator.cpp

@@ -68,8 +68,8 @@ struct RetryTest : testing::Test
 
 struct TimeoutAndRetry : testing::Test
 {
-  BT::TimeoutNode timeout_root;
   BT::RetryNode retry;
+  BT::TimeoutNode timeout_root;
   BT::SyncActionTest action;
 
   TimeoutAndRetry() : timeout_root("deadline", 9), retry("retry", 1000), action("action")