Fix TSAN errors (#4998)

Co-authored-by: Ubuntu <takurosato@microsoft.com--username>

cmake/common.cmake

@@ -138,7 +138,7 @@ install(PROGRAMS ${CCF_DIR}/samples/scripts/snpinfo.sh DESTINATION bin)
 install(FILES ${CCF_DIR}/tests/config.jinja DESTINATION bin)
 
 if(SAN)
-  install(FILES ${CCF_DIR}/src/ubsan.suppressions DESTINATION bin)
+  install(FILES ${CCF_DIR}/src/san_common.suppressions DESTINATION bin)
 endif()
 
 # Install getting_started scripts for VM creation and setup
@@ -219,6 +219,7 @@ function(add_unit_test name)
     APPEND
     PROPERTY LABELS unit_test
   )
+
 endfunction()
 
 # Test binary wrapper
@@ -618,6 +619,13 @@ function(add_e2e_test)
       )
     endif()
 
+    set_property(
+      TEST ${PARSED_ARGS_NAME}
+      APPEND
+      PROPERTY ENVIRONMENT
+               "TSAN_OPTIONS=suppressions=${CCF_DIR}/tsan_env_suppressions"
+    )
+
     set_property(
       TEST ${PARSED_ARGS_NAME}
       APPEND

cmake/tools.cmake

@@ -2,17 +2,25 @@
 # Licensed under the Apache 2.0 License.
 
 function(add_san name)
-  if(SAN)
-    # CCF_PROJECT is defined when building CCF itself, but not when this
-    # function is used by downstream applications.
-    if(CCF_PROJECT)
-      set(suppressions_file
-          $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/src/ubsan.suppressions>$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/bin/ubsan.suppressions>
-      )
-    else()
-      set(suppressions_file ${CCF_DIR}/bin/ubsan.suppressions)
-    endif()
-
+  # CCF_PROJECT is defined when building CCF itself, but not when this function
+  # is used by downstream applications.
+  if(CCF_PROJECT)
+    set(suppressions_file
+        $<BUILD_INTERFACE:${CMAKE_SOURCE_DIR}/src/san_common.suppressions>$<INSTALL_INTERFACE:$<INSTALL_PREFIX>/bin/san_common.suppressions>
+    )
+  else()
+    set(suppressions_file ${CCF_DIR}/bin/san_common.suppressions)
+  endif()
+  if(TSAN)
+    target_compile_options(
+      ${name} PRIVATE -fsanitize=thread
+                      -fsanitize-blacklist=${suppressions_file}
+    )
+    target_link_libraries(
+      ${name} PRIVATE -fsanitize=thread
+                      -fsanitize-blacklist=${suppressions_file}
+    )
+  elseif(SAN)
     target_compile_options(
       ${name}
       PRIVATE -fsanitize=undefined,address -fno-omit-frame-pointer

src/enclave/enclave_time.cpp

@@ -5,5 +5,6 @@
 namespace ccf::enclavetime
 {
   std::atomic<long long>* host_time_us = nullptr;
-  std::chrono::microseconds last_value(0);
+  std::atomic<std::chrono::microseconds> last_value(
+    std::chrono::microseconds(0));
 }

src/enclave/enclave_time.h

@@ -12,7 +12,7 @@ namespace ccf
   namespace enclavetime
   {
     extern std::atomic<long long>* host_time_us;
-    extern std::chrono::microseconds last_value;
+    extern std::atomic<std::chrono::microseconds> last_value;
   }
 
   static std::chrono::microseconds get_enclave_time()
@@ -21,7 +21,7 @@ namespace ccf
     if (enclavetime::host_time_us != nullptr)
     {
       const auto current_time = enclavetime::host_time_us->load();
-      if (current_time >= enclavetime::last_value.count())
+      if (current_time >= enclavetime::last_value.load().count())
       {
         enclavetime::last_value = std::chrono::microseconds(current_time);
       }
@@ -30,7 +30,7 @@ namespace ccf
         LOG_FAIL_FMT(
           "Host attempting to move enclave time backwards! Last value was {}, "
           "now {}",
-          enclavetime::last_value.count(),
+          enclavetime::last_value.load().count(),
           current_time);
       }
     }

src/kv/store.h

@@ -16,6 +16,7 @@
 
 #define FMT_HEADER_ONLY
 #include <fmt/format.h>
+#include <memory>
 
 namespace kv
 {
@@ -191,12 +192,15 @@ namespace kv
 
     std::shared_ptr<Consensus> get_consensus() override
     {
-      return consensus;
+      // We need to use std::atomic_load<std::shared_ptr<T>>
+      // after clang supports it.
+      // https://en.cppreference.com/w/Template:cpp/compiler_support/20
+      return std::atomic_load(&consensus);
     }
 
     void set_consensus(const std::shared_ptr<Consensus>& consensus_)
     {
-      consensus = consensus_;
+      std::atomic_store(&consensus, consensus_);
     }
 
     std::shared_ptr<TxHistory> get_history() override
@@ -938,7 +942,7 @@ namespace kv
 
       {
         std::lock_guard<ccf::pal::Mutex> vguard(version_lock);
-        if (txid.term != term_of_next_version && consensus->is_primary())
+        if (txid.term != term_of_next_version && get_consensus()->is_primary())
         {
           // This can happen when a transaction started before a view change,
           // but tries to commit after the view change is complete.
@@ -1027,7 +1031,9 @@ namespace kv
 
         replication_view = term_of_next_version;
 
-        if (consensus->type() == ConsensusType::BFT && consensus->is_backup())
+        if (
+          get_consensus()->type() == ConsensusType::BFT &&
+          get_consensus()->is_backup())
         {
           last_replicated = next_last_replicated;
         }
@@ -1039,7 +1045,8 @@ namespace kv
         if (
           last_replicated == previous_last_replicated &&
           previous_rollback_count == rollback_count &&
-          !(consensus->type() == ConsensusType::BFT && consensus->is_backup()))
+          !(get_consensus()->type() == ConsensusType::BFT &&
+            get_consensus()->is_backup()))
         {
           last_replicated = next_last_replicated;
         }

src/node/test/channels.cpp

@@ -21,7 +21,8 @@
 namespace ccf::enclavetime
 {
   std::atomic<long long>* host_time_us = nullptr;
-  std::chrono::microseconds last_value(0);
+  std::atomic<std::chrono::microseconds> last_value(
+    std::chrono::microseconds(0));
 }
 
 namespace ccf

src/san_common.suppressions

@@ -1,5 +1,5 @@
 #############################################################################
-# UBSan suppressions.
+# Common Sanitizer suppressions.
 #############################################################################
 
 #############################################################################

tests/infra/remote.py

@@ -637,6 +637,7 @@ class CCFRemote(object):
                 ubsan_opts = kwargs.get("ubsan_options")
                 if ubsan_opts:
                     env["UBSAN_OPTIONS"] += ":" + ubsan_opts
+                env["TSAN_OPTIONS"] = os.environ.get("TSAN_OPTIONS", "")
             elif enclave_platform == "snp":
                 env = snp.get_aci_env()
                 snp_security_policy_envvar = (

tsan_env_suppressions

@@ -0,0 +1,8 @@
+# For ThreadSanitizerSuppressions
+# https://github.com/google/sanitizers/wiki/ThreadSanitizerSuppressions
+
+race:*/ring_buffer.h
+
+deadlock:*/store.h
+deadlock:*/untyped_map.h
+deadlock:*/apply_changes.h