Check ubsan (#2239)

* Make UBSAN fail tests on CI

* Add no sanitize recover to ubsan

* Fix blob formatting

* Add stacktrace printing to debug failing UBSAN

* UBSAN print stacktrace

* Suppress false positive in rapidjson

---------

Co-authored-by: Harrm <[email protected]>

.github/workflows/test.yml

@@ -131,7 +131,8 @@ jobs:
           - name: "Self-hosted: Linux: clang-16 TSAN WAVM"
             run: ./housekeeping/make_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain/clang-16_cxx20.cmake -DTSAN=ON -DWASM_COMPILER=WAVM
           - name: "Self-hosted: Linux: clang-16 UBSAN"
-            run: ./housekeeping/make_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain/clang-16_cxx20.cmake -DUBSAN=ON
+            run: ./housekeeping/make_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=cmake/toolchain/clang-16_cxx20.cmake -DUBSAN=ON -DUBSAN_TRAP=OFF -DUBSAN_ABORT=ON
+            env: UBSAN_OPTIONS=print_stacktrace=1
         # Need to fix
         #  - name: "Self-hosted: Linux: clang-16 External Project"
         #    run: ./housekeeping/make_external_build.sh -DCLEAR_OBJS=ON -DCMAKE_TOOLCHAIN_FILE=../../cmake/toolchain/clang-16_cxx20.cmake

CMakeLists.txt

@@ -98,6 +98,12 @@ option(TSAN "Enable thread sanitizer" OFF)
 option(UBSAN "Enable UB sanitizer" OFF)
 # cmake-format: on
 
+# useful for tests, an error message will be printed
+option(UBSAN_ABORT "Make UB sanitizer abort on error" OFF)
+
+# useful to catch the exact wrong place with a debugger
+option(UBSAN_TRAP "Make UB sanitizer execute a trap instruction on error" OFF)
+
 set(RECOMMENDED_CLANG_FORMAT_VERSION 16)
 
 include(CheckCXXCompilerFlag)

cmake/functions.cmake

@@ -15,7 +15,7 @@ function(addtest test_name)
   set(xml_output "--gtest_output=xml:${CMAKE_BINARY_DIR}/xunit/xunit-${test_name}.xml")
   add_test(
       NAME ${test_name}
-      COMMAND $<TARGET_FILE:${test_name}> ${xml_output}
+      COMMAND $<TARGET_FILE:${test_name}> ${xml_output} "--output-on-failure"
   )
   set_target_properties(${test_name} PROPERTIES
       RUNTIME_OUTPUT_DIRECTORY ${CMAKE_BINARY_DIR}/test_bin

cmake/toolchain/flags/sanitize_undefined.cmake

@@ -10,7 +10,16 @@ set(FLAGS
     -fsanitize=undefined
     -fno-omit-frame-pointer
     -g
+    -O0
+    -fsanitize-ignorelist="${CMAKE_CURRENT_LIST_DIR}/ubsan_ignore.txt"
     )
+if (UBSAN_ABORT) 
+  list(APPEND FLAGS -fno-sanitize-recover=undefined)
+endif()
+if (UBSAN_TRAP)
+  list(APPEND FLAGS -fsanitize-trap=undefined)
+endif()
+
 foreach(FLAG IN LISTS FLAGS)
   add_cache_flag(CMAKE_CXX_FLAGS ${FLAG})
   add_cache_flag(CMAKE_C_FLAGS ${FLAG})

cmake/toolchain/flags/ubsan_ignore.txt

@@ -0,0 +1,2 @@
+# false-positive on applying non-zero offset to null pointer: the pointer is never dereferenced
+fun:_ZN9rapidjson8internal5StackINS_12CrtAllocatorEE7Reserve*

core/common/blob.hpp

@@ -89,8 +89,8 @@
   struct fmt::formatter<space_name::class_name>                                \
       : fmt::formatter<space_name::class_name::Base> {                         \
     template <typename FormatCtx>                                              \
-    auto format(const space_name::class_name &blob, FormatCtx &ctx) const      \
-        -> decltype(ctx.out()) {                                               \
+    auto format(const space_name::class_name &blob,                            \
+                FormatCtx &ctx) const -> decltype(ctx.out()) {                 \
       return fmt::formatter<space_name::class_name::Base>::format(blob, ctx);  \
     }                                                                          \
   };
@@ -269,19 +269,17 @@ struct fmt::formatter<kagome::common::Blob<N>> {
   // Formats the Blob using the parsed format specification (presentation)
   // stored in this formatter.
   template <typename FormatContext>
-  auto format(const kagome::common::Blob<N> &blob, FormatContext &ctx) const
-      -> decltype(ctx.out()) {
-    // ctx.out() is an output iterator to write to.
-
+  auto format(const kagome::common::Blob<N> &blob,
+              FormatContext &ctx) const -> decltype(ctx.out()) {
     if (presentation == 's') {
-      return fmt::format_to(
-          ctx.out(),
-          "0x{:04x}…{:04x}",
-          // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
-          htobe16(*reinterpret_cast<const uint16_t *>(blob.data())),
-          // NOLINTNEXTLINE(cppcoreguidelines-pro-type-reinterpret-cast)
-          htobe16(*reinterpret_cast<const uint16_t *>(blob.data() + blob.size()
-                                                      - sizeof(uint16_t))));
+      if constexpr (N > 4) {
+        uint16_t head = static_cast<uint16_t>(blob[1])
+                      | (static_cast<uint16_t>(blob[0]) << 8);
+        uint16_t tail = static_cast<uint16_t>(blob[blob.size() - 1])
+                      | (static_cast<uint16_t>(blob[blob.size() - 2]) << 8);
+        return fmt::format_to(ctx.out(), "0x{:04x}…{:04x}", head, tail);
+      }
+      // else fallback to normal print
     }
 
     return fmt::format_to(ctx.out(), "0x{}", blob.toHex());

core/runtime/memory.hpp

@@ -112,6 +112,9 @@ namespace kagome::runtime {
     }
 
     void storeBuffer(WasmPointer ptr, common::BufferView v) {
+      if (v.empty()) {
+        return;
+      }
       memcpy(handle_->view(ptr, v.size()).value().data(), v.data(), v.size());
     }
 

core/storage/trie/polkadot_trie/polkadot_trie_impl.cpp

@@ -319,17 +319,17 @@ namespace kagome::storage::trie {
 
   PolkadotTrieImpl::PolkadotTrieImpl(RetrieveFunctions retrieve_functions)
       : nodes_{std::make_unique<OpaqueNodeStorage>(
-          std::move(retrieve_functions.retrieve_node),
-          std::move(retrieve_functions.retrieve_value),
-          nullptr)},
+            std::move(retrieve_functions.retrieve_node),
+            std::move(retrieve_functions.retrieve_value),
+            nullptr)},
         logger_{log::createLogger("PolkadotTrie", "trie")} {}
 
   PolkadotTrieImpl::PolkadotTrieImpl(NodePtr root,
                                      RetrieveFunctions retrieve_functions)
       : nodes_{std::make_unique<OpaqueNodeStorage>(
-          std::move(retrieve_functions.retrieve_node),
-          std::move(retrieve_functions.retrieve_value),
-          std::move(root))},
+            std::move(retrieve_functions.retrieve_node),
+            std::move(retrieve_functions.retrieve_value),
+            std::move(root))},
         logger_{log::createLogger("PolkadotTrie", "trie")} {}
 
   //  PolkadotTrieImpl::~PolkadotTrieImpl() {}
@@ -561,7 +561,9 @@ namespace kagome::storage::trie {
       auto parent_as_branch =
           std::dynamic_pointer_cast<const BranchNode>(parent);
       OUTCOME_TRY(child, retrieveChild(*parent_as_branch, path[common_length]));
-      OUTCOME_TRY(callback(*parent_as_branch, path[common_length], *child));
+      if (child != nullptr) {
+        OUTCOME_TRY(callback(*parent_as_branch, path[common_length], *child));
+      }
       return forNodeInPath(child, path.subspan(common_length + 1), callback);
     }
     if (parent->getKeyNibbles() == path) {

test/core/application/app_config_test.cpp

@@ -144,7 +144,7 @@ class AppConfigurationTest : public testing::Test {
  * @then only default values are available
  */
 TEST_F(AppConfigurationTest, DefaultValuesTest) {
-  boost::asio::ip::tcp::endpoint const ws_endpoint =
+  const boost::asio::ip::tcp::endpoint ws_endpoint =
       get_endpoint("0.0.0.0", 9944);
   const char *args[] = {"/path/",
                         "--chain",
@@ -165,7 +165,7 @@ TEST_F(AppConfigurationTest, DefaultValuesTest) {
  * @then we must receive correct endpoints on call
  */
 TEST_F(AppConfigurationTest, EndpointsTest) {
-  boost::asio::ip::tcp::endpoint const ws_endpoint =
+  const boost::asio::ip::tcp::endpoint ws_endpoint =
       get_endpoint("5.6.7.8", 2222);
   const char *args[] = {
       "/path/",
@@ -208,7 +208,7 @@ TEST_F(AppConfigurationTest, GenesisPathTest) {
  * @then we must select cmd line version
  */
 TEST_F(AppConfigurationTest, CrossConfigTest) {
-  boost::asio::ip::tcp::endpoint const ws_endpoint =
+  const boost::asio::ip::tcp::endpoint ws_endpoint =
       get_endpoint("5.6.7.8", 2222);
   const char *args[] = {
       "/path/",
@@ -303,7 +303,7 @@ TEST_F(AppConfigurationTest, RocksDBStorageBackend) {
  * @then we must put to config data from file
  */
 TEST_F(AppConfigurationTest, ConfigFileTest) {
-  boost::asio::ip::tcp::endpoint const ws_endpoint =
+  const boost::asio::ip::tcp::endpoint ws_endpoint =
       get_endpoint("2.2.2.2", 3456);
 
   const char *args[] = {"/path/", "--config-file", config_path.c_str()};
@@ -328,7 +328,7 @@ TEST_F(AppConfigurationTest, ConfigFileTest) {
  * @then we must receive default values
  */
 TEST_F(AppConfigurationTest, InvalidConfigFileTest) {
-  boost::asio::ip::tcp::endpoint const ws_endpoint =
+  const boost::asio::ip::tcp::endpoint ws_endpoint =
       get_endpoint("0.0.0.0", 9944);
 
   const char *args[] = {"/path/",
@@ -356,7 +356,7 @@ TEST_F(AppConfigurationTest, InvalidConfigFileTest) {
  * @then we must receive default values
  */
 TEST_F(AppConfigurationTest, DamagedConfigFileTest) {
-  boost::asio::ip::tcp::endpoint const ws_endpoint =
+  const boost::asio::ip::tcp::endpoint ws_endpoint =
       get_endpoint("0.0.0.0", 9944);
 
   const char *args[] = {"/path/",
@@ -384,7 +384,7 @@ TEST_F(AppConfigurationTest, DamagedConfigFileTest) {
  * @then we must receive default values
  */
 TEST_F(AppConfigurationTest, NoConfigFileTest) {
-  boost::asio::ip::tcp::endpoint const ws_endpoint =
+  const boost::asio::ip::tcp::endpoint ws_endpoint =
       get_endpoint("0.0.0.0", 9944);
 
   const char *args[] = {"/path/",

test/core/authorship/block_builder_test.cpp

@@ -7,7 +7,11 @@
 #include "authorship/impl/block_builder_impl.hpp"
 
 #include <gtest/gtest.h>
+#include <qtils/outcome.hpp>
+#include "gmock/gmock.h"
 #include "mock/core/runtime/block_builder_api_mock.hpp"
+#include "mock/core/runtime/module_instance_mock.hpp"
+#include "runtime/runtime_context.hpp"
 #include "testutil/literals.hpp"
 #include "testutil/outcome.hpp"
 #include "testutil/prepare_loggers.hpp"
@@ -30,6 +34,9 @@ using kagome::primitives::Extrinsic;
 using kagome::primitives::InherentData;
 using kagome::primitives::dispatch_error::Other;
 using kagome::runtime::BlockBuilderApiMock;
+using kagome::runtime::ModuleInstanceMock;
+using kagome::runtime::RuntimeContext;
+using kagome::runtime::RuntimeContextFactory;
 using kagome::storage::trie::RootHash;
 
 class BlockBuilderTest : public ::testing::Test {
@@ -46,8 +53,14 @@ class BlockBuilderTest : public ::testing::Test {
 
     parent_block_ = BlockInfo{block_number_ - 1, expected_header_.parent_hash};
 
+    auto instance_mock = std::make_shared<ModuleInstanceMock>();
+    EXPECT_CALL(*instance_mock, stateless())
+        .WillOnce(Return(outcome::success()));
     block_builder_ = std::make_shared<BlockBuilderImpl>(
-        expected_header_, nullptr, block_builder_api_);
+        expected_header_,
+        std::make_unique<RuntimeContext>(
+            RuntimeContextFactory::stateless(instance_mock).value()),
+        block_builder_api_);
   }
 
  protected:

test/core/crypto/vrf/vrf_provider_test.cpp

@@ -115,7 +115,7 @@ TEST_F(VRFProviderTest, SignAndVerifyTranscriptSuccess) {
  */
 TEST_F(VRFProviderTest, TranscriptSignFailed) {
   // given
-  VRFThreshold threshold{std::numeric_limits<VRFPreOutput>::min()};
+  VRFThreshold threshold{std::numeric_limits<VRFThreshold>::min()};
   Transcript t_src;
   prepare_transcript(t_src);
 
@@ -181,7 +181,7 @@ TEST_F(VRFProviderTest, VerifyFailed) {
  */
 TEST_F(VRFProviderTest, SignFailed) {
   // given
-  VRFThreshold threshold{std::numeric_limits<VRFPreOutput>::min()};
+  VRFThreshold threshold{std::numeric_limits<VRFThreshold>::min()};
 
   // when
   Buffer src(reference_data);