Merge pull request #2553 from div72/sanitizers

build: add option for sanitizers

.github/workflows/ci.yml

@@ -23,6 +23,11 @@ jobs:
                     script-id: native
                   - name: x86_64 Linux  [GOAL install]  [GUI]  [bionic]  [no depends]
                     script-id: native_old
+                  - name: x86_64 Linux  [ASan]  [LSan]  [UBSan]  [integer]  [jammy]  [no depends]
+                    script-id: native_asan
+                  # FIXME: depends is unable to compile Qt with clang.
+                  # - name: x86_64 Linux  [TSan]  [GUI]  [jammy]
+                  #  script-id: native_tsan
                   - name: macOS 10.14  [GOAL deploy]  [GUI]  [no tests]  [focal]
                     script-id: mac
         env:

ci/test/00_setup_env_native_asan.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+#
+# Copyright (c) 2019-2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or https://opensource.org/licenses/mit-license.php.
+
+export LC_ALL=C.UTF-8
+
+export CONTAINER_NAME=ci_native_asan
+export PACKAGES="clang llvm libqt5gui5 libqt5core5a qtbase5-dev libqt5dbus5 qttools5-dev qttools5-dev-tools libssl-dev libevent-dev bsdmainutils libboost-system-dev libboost-filesystem-dev libboost-iostreams-dev libboost-test-dev libboost-thread-dev libdb5.3++-dev libminiupnpc-dev libqrencode-dev libzip-dev zlib1g zlib1g-dev libcurl4 libcurl4-openssl-dev"
+export DOCKER_NAME_TAG=ubuntu:22.04
+export NO_DEPENDS=1
+export GOAL="install"
+export GRIDCOIN_CONFIG="--with-incompatible-bdb --with-gui=qt5 CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER' --with-sanitizers=address,integer,undefined CC=clang CXX=clang++"

ci/test/00_setup_env_native_tsan.sh

@@ -0,0 +1,14 @@
+#!/usr/bin/env bash
+#
+# Copyright (c) 2019-2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or https://opensource.org/licenses/mit-license.php.
+
+export LC_ALL=C.UTF-8
+
+export CONTAINER_NAME=ci_native_tsan
+export DOCKER_NAME_TAG=ubuntu:22.04
+export PACKAGES="clang-13 llvm-13 libc++abi-13-dev libc++-13-dev"
+export DEP_OPTS="CC=clang-13 CXX='clang++-13 -stdlib=libc++'"
+export GOAL="install"
+export GRIDCOIN_CONFIG="CPPFLAGS='-DARENA_DEBUG -DDEBUG_LOCKORDER -DDEBUG_LOCKCONTENTION' CXXFLAGS='-g' --with-sanitizers=thread CC=clang-13 CXX='clang++-13 -stdlib=libc++'"

ci/test/04_install.sh

@@ -17,6 +17,11 @@ fi
 mkdir -p "${CCACHE_DIR}"
 mkdir -p "${PREVIOUS_RELEASES_DIR}"
 
+export ASAN_OPTIONS="detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1"
+export LSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/lsan"
+export TSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/tsan:halt_on_error=1:log_path=${BASE_SCRATCH_DIR}/sanitizer-output/tsan"
+export UBSAN_OPTIONS="suppressions=${BASE_ROOT_DIR}/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1"
+
 env | grep -E '^(GRIDCOIN_CONFIG|BASE_|QEMU_|CCACHE_|LC_ALL|BOOST_TEST_RANDOM|DEBIAN_FRONTEND|CONFIG_SHELL|(ASAN|LSAN|TSAN|UBSAN)_OPTIONS|PREVIOUS_RELEASES_DIR)' | tee /tmp/env
 if [[ $HOST = *-mingw32 ]]; then
   DOCKER_ADMIN="--cap-add SYS_ADMIN"

configure.ac

@@ -209,6 +209,12 @@ AC_ARG_ENABLE([debug],
     [enable_debug=$enableval],
     [enable_debug=no])
 
+dnl Enable different -fsanitize options
+AC_ARG_WITH([sanitizers],
+    [AS_HELP_STRING([--with-sanitizers],
+                    [comma separated list of extra sanitizers to build with (default is none enabled)])],
+    [use_sanitizers=$withval])
+
 # Turn warnings into errors
 AC_ARG_ENABLE([werror],
     [AS_HELP_STRING([--enable-werror],
@@ -230,6 +236,33 @@ if test "$enable_debug" = "yes"; then
     fi
 fi
 
+if test "$use_sanitizers" != ""; then
+  dnl First check if the compiler accepts flags. If an incompatible pair like
+  dnl -fsanitize=address,thread is used here, this check will fail. This will also
+  dnl fail if a bad argument is passed, e.g. -fsanitize=undfeined
+  AX_CHECK_COMPILE_FLAG(
+    [-fsanitize=$use_sanitizers],
+    [SANITIZER_CXXFLAGS="-fsanitize=$use_sanitizers"],
+    [AC_MSG_ERROR([compiler did not accept requested flags])])
+
+  dnl Some compilers (e.g. GCC) require additional libraries like libasan,
+  dnl libtsan, libubsan, etc. Make sure linking still works with the sanitize
+  dnl flag. This is a separate check so we can give a better error message when
+  dnl the sanitize flags are supported by the compiler but the actual sanitizer
+  dnl libs are missing.
+  AX_CHECK_LINK_FLAG(
+    [-fsanitize=$use_sanitizers],
+    [SANITIZER_LDFLAGS="-fsanitize=$use_sanitizers"],
+    [AC_MSG_ERROR([linker did not accept requested flags, you are missing required libraries])],
+    [],
+    [AC_LANG_PROGRAM([[
+    #include <cstdint>
+    #include <cstddef>
+    extern "C" int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size) { return 0; }
+    __attribute__((weak)) // allow for libFuzzer linking
+    ]],[[]])])
+fi
+
 ERROR_CXXFLAGS=
 if test "$enable_werror" = "yes"; then
   if test "$CXXFLAG_WERROR" = ""; then
@@ -1312,6 +1345,8 @@ AC_SUBST(HARDENED_CPPFLAGS)
 AC_SUBST(HARDENED_LDFLAGS)
 AC_SUBST(PIC_FLAGS)
 AC_SUBST(PIE_FLAGS)
+AC_SUBST(SANITIZER_CXXFLAGS)
+AC_SUBST(SANITIZER_LDFLAGS)
 AC_SUBST(SSE42_CXXFLAGS)
 AC_SUBST(SSE41_CXXFLAGS)
 AC_SUBST(AVX2_CXXFLAGS)
@@ -1418,6 +1453,7 @@ echo "  with zmq      = $use_zmq"
 echo "  with test     = $use_tests"
 echo "  with bench    = $use_bench"
 echo "  with upnp     = $use_upnp"
+echo "  sanitizers    = $use_sanitizers"
 echo "  debug enabled = $enable_debug"
 echo "  werror        = $enable_werror"
 echo

src/Makefile.am

@@ -8,8 +8,8 @@ print-%: FORCE
 
 DIST_SUBDIRS = univalue secp256k1
 
-AM_LDFLAGS = ${libcurl_LIBS} $(PTHREAD_CFLAGS) $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS)
-AM_CXXFLAGS = $(HARDENED_CXXFLAGS) $(ERROR_CXXFLAGS)
+AM_LDFLAGS = ${libcurl_LIBS} $(PTHREAD_CFLAGS) $(LIBTOOL_LDFLAGS) $(HARDENED_LDFLAGS) $(SANITIZER_LDFLAGS)
+AM_CXXFLAGS = $(HARDENED_CXXFLAGS) $(ERROR_CXXFLAGS) $(SANITIZER_CXXFLAGS)
 AM_CPPFLAGS = ${libcurl_CFLAGS} $(HARDENED_CPPFLAGS) -DSTATICLIB -DCURL_STATICLIB -DMINIUPNP_STATICLIB -DZIP_STATIC -DNN_STATIC_LIB
 EXTRA_LIBRARIES =
 

src/consensus/merkle.cpp

@@ -37,118 +37,25 @@
        root.
 */
 
-/* This implements a constant-space merkle root/path calculator, limited to 2^32 leaves. */
-static void MerkleComputation(const std::vector<uint256>& leaves, uint256* proot, bool* pmutated, uint32_t branchpos, std::vector<uint256>* pbranch) {
-    if (pbranch) pbranch->clear();
-    if (leaves.size() == 0) {
-        if (pmutated) *pmutated = false;
-        if (proot) *proot = uint256();
-        return;
-    }
-    bool mutated = false;
-    // count is the number of leaves processed so far.
-    uint32_t count = 0;
-    // inner is an array of eagerly computed subtree hashes, indexed by tree
-    // level (0 being the leaves).
-    // For example, when count is 25 (11001 in binary), inner[4] is the hash of
-    // the first 16 leaves, inner[3] of the next 8 leaves, and inner[0] equal to
-    // the last leaf. The other inner entries are undefined.
-    uint256 inner[32];
-    // Which position in inner is a hash that depends on the matching leaf.
-    int matchlevel = -1;
-    // First process all leaves into 'inner' values.
-    while (count < leaves.size()) {
-        uint256 h = leaves[count];
-        bool matchh = count == branchpos;
-        count++;
-        int level;
-        // For each of the lower bits in count that are 0, do 1 step. Each
-        // corresponds to an inner value that existed before processing the
-        // current leaf, and each needs a hash to combine it.
-        for (level = 0; !(count & (((uint32_t)1) << level)); level++) {
-            if (pbranch) {
-                if (matchh) {
-                    pbranch->push_back(inner[level]);
-                } else if (matchlevel == level) {
-                    pbranch->push_back(h);
-                    matchh = true;
-                }
+uint256 ComputeMerkleRoot(std::vector<uint256> hashes, bool* mutated) {
+    bool mutation = false;
+    while (hashes.size() > 1) {
+        if (mutated) {
+            for (size_t pos = 0; pos + 1 < hashes.size(); pos += 2) {
+                if (hashes[pos] == hashes[pos + 1]) mutation = true;
             }
-            mutated |= (inner[level] == h);
-            CHash256().Write(inner[level]).Write(h).Finalize(h);
-        }
-        // Store the resulting hash at inner position level.
-        inner[level] = h;
-        if (matchh) {
-            matchlevel = level;
-        }
-    }
-    // Do a final 'sweep' over the rightmost branch of the tree to process
-    // odd levels, and reduce everything to a single top value.
-    // Level is the level (counted from the bottom) up to which we've sweeped.
-    int level = 0;
-    // As long as bit number level in count is zero, skip it. It means there
-    // is nothing left at this level.
-    while (!(count & (((uint32_t)1) << level))) {
-        level++;
-    }
-    uint256 h = inner[level];
-    bool matchh = matchlevel == level;
-    while (count != (((uint32_t)1) << level)) {
-        // If we reach this point, h is an inner value that is not the top.
-        // We combine it with itself (Bitcoin's special rule for odd levels in
-        // the tree) to produce a higher level one.
-        if (pbranch && matchh) {
-            pbranch->push_back(h);
         }
-        CHash256().Write(h).Write(h).Finalize(h);
-        // Increment count to the value it would have if two entries at this
-        // level had existed.
-        count += (((uint32_t)1) << level);
-        level++;
-        // And propagate the result upwards accordingly.
-        while (!(count & (((uint32_t)1) << level))) {
-            if (pbranch) {
-                if (matchh) {
-                    pbranch->push_back(inner[level]);
-                } else if (matchlevel == level) {
-                    pbranch->push_back(h);
-                    matchh = true;
-                }
-            }
-            CHash256().Write(inner[level]).Write(h).Finalize(h);
-            level++;
+        if (hashes.size() & 1) {
+            hashes.push_back(hashes.back());
         }
+        SHA256D64(hashes[0].begin(), hashes[0].begin(), hashes.size() / 2);
+        hashes.resize(hashes.size() / 2);
     }
-    // Return result.
-    if (pmutated) *pmutated = mutated;
-    if (proot) *proot = h;
-}
-
-uint256 ComputeMerkleRoot(const std::vector<uint256>& leaves, bool* mutated) {
-    uint256 hash;
-    MerkleComputation(leaves, &hash, mutated, -1, nullptr);
-    return hash;
-}
-
-std::vector<uint256> ComputeMerkleBranch(const std::vector<uint256>& leaves, uint32_t position) {
-    std::vector<uint256> ret;
-    MerkleComputation(leaves, nullptr, nullptr, position, &ret);
-    return ret;
+    if (mutated) *mutated = mutation;
+    if (hashes.size() == 0) return uint256();
+    return hashes[0];
 }
 
-uint256 ComputeMerkleRootFromBranch(const uint256& leaf, const std::vector<uint256>& vMerkleBranch, uint32_t nIndex) {
-    uint256 hash = leaf;
-    for (std::vector<uint256>::const_iterator it = vMerkleBranch.begin(); it != vMerkleBranch.end(); ++it) {
-        if (nIndex & 1) {
-            hash = Hash(*it, hash);
-        } else {
-            hash = Hash(hash, *it);
-        }
-        nIndex >>= 1;
-    }
-    return hash;
-}
 
 uint256 BlockMerkleRoot(const CBlock& block, bool* mutated)
 {
@@ -157,15 +64,6 @@ uint256 BlockMerkleRoot(const CBlock& block, bool* mutated)
     for (size_t s = 0; s < block.vtx.size(); s++) {
         leaves[s] = block.vtx[s].GetHash();
     }
-    return ComputeMerkleRoot(leaves, mutated);
+    return ComputeMerkleRoot(std::move(leaves), mutated);
 }
 
-std::vector<uint256> BlockMerkleBranch(const CBlock& block, uint32_t position)
-{
-    std::vector<uint256> leaves;
-    leaves.resize(block.vtx.size());
-    for (size_t s = 0; s < block.vtx.size(); s++) {
-        leaves[s] = block.vtx[s].GetHash();
-    }
-    return ComputeMerkleBranch(leaves, position);
-}

src/consensus/merkle.h

@@ -5,27 +5,17 @@
 #ifndef BITCOIN_CONSENSUS_MERKLE_H
 #define BITCOIN_CONSENSUS_MERKLE_H
 
-#include <stdint.h>
 #include <vector>
 
 #include "main.h"
 #include <uint256.h>
 
-uint256 ComputeMerkleRoot(const std::vector<uint256>& leaves, bool* mutated = nullptr);
-std::vector<uint256> ComputeMerkleBranch(const std::vector<uint256>& leaves, uint32_t position);
-uint256 ComputeMerkleRootFromBranch(const uint256& leaf, const std::vector<uint256>& branch, uint32_t position);
+uint256 ComputeMerkleRoot(std::vector<uint256> leaves, bool* mutated = nullptr);
 
 /*
  * Compute the Merkle root of the transactions in a block.
  * *mutated is set to true if a duplicated subtree was found.
  */
 uint256 BlockMerkleRoot(const CBlock& block, bool* mutated = nullptr);
 
-/*
- * Compute the Merkle branch for the tree of transactions in a block, for a
- * given position.
- * This can be verified using ComputeMerkleRootFromBranch.
- */
-std::vector<uint256> BlockMerkleBranch(const CBlock& block, uint32_t position);
-
 #endif // BITCOIN_CONSENSUS_MERKLE_H

src/gridcoin/cpid.h

@@ -454,9 +454,9 @@ namespace std {
 //! This enables the use of GRC::Cpid as a key in a std::unordered_map object.
 //!
 //! CONSENSUS: Don't use the hash produced by this routine (or by any std::hash
-//! specialization) in protocol-specific implementations. It ignores endianness
-//! and outputs a value with a chance of collision probably too great for usage
-//! besides the intended local look-up functionality.
+//! specialization) in protocol-specific implementations. It outputs a value
+//! with a chance of collision probably too great for usage besides the intended
+//! local look-up functionality.
 //!
 template<>
 struct hash<GRC::Cpid>
@@ -473,8 +473,16 @@ struct hash<GRC::Cpid>
         // Just convert the CPID into a value that we can store in a size_t
         // object. CPIDs are already unique identifiers.
         //
-        return *reinterpret_cast<const uint64_t*>(cpid.Raw().data())
-            + *reinterpret_cast<const uint64_t*>(cpid.Raw().data() + 8);
+        const auto& data = cpid.Raw();
+        size_t ret = ((size_t)(data[0] & 255)       | (size_t)(data[1] & 255) << 8 |
+                      (size_t)(data[2] & 255) << 16 | (size_t)(data[3] & 255) << 24);
+
+        if (sizeof(size_t) == 8) {
+            ret |= ((size_t)(data[4] & 255) << 32 | (size_t)(data[5] & 255) << 40 |
+                    (size_t)(data[6] & 255) << 48 | (size_t)(data[7] & 255) << 56);
+        }
+
+        return ret;
     }
 };
 }

src/gridcoin/mrc.h

@@ -125,14 +125,14 @@ class MRC : public IContractPayload
     //! incoming reward claims and can index those calculated values without
     //! this field. It can be considered informational.
     //!
-    CAmount m_research_subsidy;
+    CAmount m_research_subsidy = 0;
 
     //!
     //! \brief The value of the fees charged to the MRC claimant. These will be
     //! subtracted from the research subsidy and distributed to the staker and
     //! the foundation according to protocol rules encapsulated in ComputeMRCFee().
     //!
-    CAmount m_fee;
+    CAmount m_fee = 0;
 
     //!
     //! \brief The researcher magnitude value from the superblock at the time
@@ -145,14 +145,14 @@ class MRC : public IContractPayload
     //!
     //! Previous protocol versions used the magnitude in reward calculations.
     //!
-    uint16_t m_magnitude;
+    uint16_t m_magnitude = 0;
 
     //!
     //! \brief The magnitude ratio of the network at the time of the claim.
     //!
     //! Informational.
     //!
-    double m_magnitude_unit;
+    double m_magnitude_unit = 0.0;
 
     //!
     //! \brief The hash of the last block (head of the chain) for the MRC

src/gridcoin/scraper/scraper_net.cpp

@@ -139,9 +139,7 @@ int CSplitBlob::addPartData(CDataStream&& vData)
     {
         /* missing data; use the supplied data */
         /* prevent calling the Complete callback FIXME: make this look better */
-        cntPartsRcvd--;
         CSplitBlob::RecvPart(nullptr, vData);
-        cntPartsRcvd++;
     }
     return n;
 }

src/gridcoin/superblock.cpp

@@ -456,8 +456,9 @@ class LegacySuperblockParser
 
         for (size_t x = 0; x < binary_size && binary_size - x >= 18; x += 18) {
             magnitudes.AddLegacy(
-                *reinterpret_cast<const Cpid*>(byte_ptr + x),
-                be16toh(*reinterpret_cast<const int16_t*>(byte_ptr + x + 16)));
+                Cpid(std::vector<unsigned char>(byte_ptr + x, byte_ptr + x + 16)),
+                (((uint16_t)byte_ptr[x + 16] & 0xFF) << 8) | ((uint16_t)byte_ptr[x + 17] & 0xFF)
+            );
         }
 
         return magnitudes;