From 9dd2ca1e4a358937ad29d46832f172152fa037d6 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Thu, 19 Mar 2026 12:51:51 +0000 Subject: [PATCH] fix: Address code review findings for TSAN/ASAN PR - Fix BasicApp::startIOThreads() member corruption: replace while(numberOfThreads_--) with a for loop to preserve the member value. - Fix Coro.ipp non-sanitizer stack size: increase default from 1MB to 2MB to match yield_to.h and prevent stack overflows in deep call chains. - Remove stale "TSAN deactivated" comment and dead activate_tsan variable from CI matrix generator. - Clarify Application.cpp setup() comment to distinguish io_context threads from subsystem-specific threads. - Use explicit load(std::memory_order_relaxed) at all SHAMap::ledgerSeq_ read sites for consistency with the atomic store. - Extract sanitizer detection into XRPL_SANITIZER_ACTIVE macro in sanitizers.h, replacing duplicated preprocessor blocks in Coro.ipp and yield_to.h. Co-Authored-By: Claude Opus 4.6 --- .github/scripts/strategy-matrix/generate.py | 28 ++++++++++----------- include/xrpl/basics/sanitizers.h | 25 +++++++++++++----- include/xrpl/beast/test/yield_to.h | 15 +++-------- include/xrpl/core/Coro.ipp | 13 ++-------- src/libxrpl/shamap/SHAMap.cpp | 21 ++++++++++++---- src/libxrpl/shamap/SHAMapSync.cpp | 12 +++++++-- src/xrpld/app/main/Application.cpp | 10 +++----- src/xrpld/app/main/BasicApp.cpp | 4 +-- 8 files changed, 70 insertions(+), 58 deletions(-) diff --git a/.github/scripts/strategy-matrix/generate.py b/.github/scripts/strategy-matrix/generate.py index 0d99995829..8284a1ee96 100755 --- a/.github/scripts/strategy-matrix/generate.py +++ b/.github/scripts/strategy-matrix/generate.py @@ -255,21 +255,19 @@ def generate_strategy_matrix(all: bool, config: Config) -> list: "sanitizers": "address,undefinedbehavior", } ) - # TSAN is deactivated due to seg faults with latest compilers. - activate_tsan = True - if activate_tsan: - configurations.append( - { - "config_name": config_name + "-tsan-ubsan", - "cmake_args": cmake_args, - "cmake_target": cmake_target, - "build_only": build_only, - "build_type": build_type, - "os": os, - "architecture": architecture, - "sanitizers": "thread,undefinedbehavior", - } - ) + # Add TSAN + UBSAN configuration. + configurations.append( + { + "config_name": config_name + "-tsan-ubsan", + "cmake_args": cmake_args, + "cmake_target": cmake_target, + "build_only": build_only, + "build_type": build_type, + "os": os, + "architecture": architecture, + "sanitizers": "thread,undefinedbehavior", + } + ) else: configurations.append( { diff --git a/include/xrpl/basics/sanitizers.h b/include/xrpl/basics/sanitizers.h index b954952848..c1b8d4c431 100644 --- a/include/xrpl/basics/sanitizers.h +++ b/include/xrpl/basics/sanitizers.h @@ -1,13 +1,26 @@ #pragma once -// Helper to disable ASan/HwASan for specific functions -/* - ASAN flags some false positives with sudden jumps in control flow, like - exceptions, or when encountering coroutine stack switches. This macro can be used to disable ASAN - intrumentation for specific functions. -*/ +// Helper to disable ASan/HwASan for specific functions. +// ASAN flags some false positives with sudden jumps in control flow, like +// exceptions, or when encountering coroutine stack switches. This macro can +// be used to disable ASAN instrumentation for specific functions. #if defined(__GNUC__) || defined(__clang__) #define XRPL_NO_SANITIZE_ADDRESS __attribute__((no_sanitize("address", "hwaddress"))) #else #define XRPL_NO_SANITIZE_ADDRESS #endif + +// Detect whether a memory sanitizer (TSAN or ASAN) is active at compile time. +// GCC defines __SANITIZE_THREAD__ / __SANITIZE_ADDRESS__ directly. +// Clang uses __has_feature(thread_sanitizer) / __has_feature(address_sanitizer). +#if defined(__SANITIZE_THREAD__) || defined(__SANITIZE_ADDRESS__) +#define XRPL_SANITIZER_ACTIVE 1 +#elif defined(__has_feature) +#if __has_feature(thread_sanitizer) || __has_feature(address_sanitizer) +#define XRPL_SANITIZER_ACTIVE 1 +#endif +#endif + +#ifndef XRPL_SANITIZER_ACTIVE +#define XRPL_SANITIZER_ACTIVE 0 +#endif diff --git a/include/xrpl/beast/test/yield_to.h b/include/xrpl/beast/test/yield_to.h index 21135f3ab9..797da0be16 100644 --- a/include/xrpl/beast/test/yield_to.h +++ b/include/xrpl/beast/test/yield_to.h @@ -4,6 +4,8 @@ #pragma once +#include + #include #include #include @@ -21,17 +23,8 @@ namespace test { // Sanitizers significantly increase stack frame sizes // (TSAN ~3-5x, ASAN ~2-3x), requiring larger coroutine stacks. -#if defined(__SANITIZE_THREAD__) || defined(__SANITIZE_ADDRESS__) -inline constexpr std::size_t yieldStackSize = 4 * 1024 * 1024; -#elif defined(__has_feature) -#if __has_feature(thread_sanitizer) || __has_feature(address_sanitizer) -inline constexpr std::size_t yieldStackSize = 4 * 1024 * 1024; -#else -inline constexpr std::size_t yieldStackSize = 2 * 1024 * 1024; -#endif -#else -inline constexpr std::size_t yieldStackSize = 2 * 1024 * 1024; -#endif +inline constexpr std::size_t yieldStackSize = + XRPL_SANITIZER_ACTIVE ? 4 * 1024 * 1024 : 2 * 1024 * 1024; /** Mix-in to support tests using asio coroutines. diff --git a/include/xrpl/core/Coro.ipp b/include/xrpl/core/Coro.ipp index da172f1c28..9791c0befd 100644 --- a/include/xrpl/core/Coro.ipp +++ b/include/xrpl/core/Coro.ipp @@ -1,6 +1,7 @@ #pragma once #include +#include #include @@ -8,17 +9,7 @@ namespace xrpl { // Sanitizers significantly increase stack frame sizes // (TSAN ~3-5x, ASAN ~2-3x), requiring larger coroutine stacks. -#if defined(__SANITIZE_THREAD__) || defined(__SANITIZE_ADDRESS__) -inline constexpr std::size_t coroStackSize = megabytes(4); -#elif defined(__has_feature) -#if __has_feature(thread_sanitizer) || __has_feature(address_sanitizer) -inline constexpr std::size_t coroStackSize = megabytes(4); -#else -inline constexpr std::size_t coroStackSize = megabytes(1); -#endif -#else -inline constexpr std::size_t coroStackSize = megabytes(1); -#endif +inline constexpr std::size_t coroStackSize = XRPL_SANITIZER_ACTIVE ? megabytes(4) : megabytes(2); template JobQueue::Coro::Coro(Coro_create_t, JobQueue& jq, JobType type, std::string const& name, F&& f) diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index c757e1c7d0..ab230a5ae1 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -139,7 +139,8 @@ intr_ptr::SharedPtr SHAMap::fetchNodeFromDB(SHAMapHash const& hash) const { XRPL_ASSERT(backed_, "xrpl::SHAMap::fetchNodeFromDB : is backed"); - auto obj = f_.db().fetchNodeObject(hash.as_uint256(), ledgerSeq_); + auto obj = + f_.db().fetchNodeObject(hash.as_uint256(), ledgerSeq_.load(std::memory_order_relaxed)); return finishFetch(hash, obj); } @@ -155,7 +156,8 @@ SHAMap::finishFetch(SHAMapHash const& hash, std::shared_ptr const& o if (full_) { full_ = false; - f_.missingNodeAcquireBySeq(ledgerSeq_, hash.as_uint256()); + f_.missingNodeAcquireBySeq( + ledgerSeq_.load(std::memory_order_relaxed), hash.as_uint256()); } return {}; } @@ -188,7 +190,12 @@ SHAMap::checkFilter(SHAMapHash const& hash, SHAMapSyncFilter* filter) const auto node = SHAMapTreeNode::makeFromPrefix(makeSlice(*nodeData), hash); if (node) { - filter->gotNode(true, hash, ledgerSeq_, std::move(*nodeData), node->getType()); + filter->gotNode( + true, + hash, + ledgerSeq_.load(std::memory_order_relaxed), + std::move(*nodeData), + node->getType()); if (backed_) canonicalize(hash, node); } @@ -369,7 +376,7 @@ SHAMap::descendAsync( { f_.db().asyncFetch( hash.as_uint256(), - ledgerSeq_, + ledgerSeq_.load(std::memory_order_relaxed), [this, hash, cb{std::move(callback)}](std::shared_ptr const& object) { auto node = finishFetch(hash, object); cb(node, hash); @@ -911,7 +918,11 @@ SHAMap::writeNode(NodeObjectType t, intr_ptr::SharedPtr node) co Serializer s; node->serializeWithPrefix(s); - f_.db().store(t, std::move(s.modData()), node->getHash().as_uint256(), ledgerSeq_); + f_.db().store( + t, + std::move(s.modData()), + node->getHash().as_uint256(), + ledgerSeq_.load(std::memory_order_relaxed)); return node; } diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 948d972c18..82a71652e9 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -510,7 +510,11 @@ SHAMap::addRootNode(SHAMapHash const& hash, Slice const& rootNode, SHAMapSyncFil Serializer s; root_->serializeWithPrefix(s); filter->gotNode( - false, root_->getHash(), ledgerSeq_, std::move(s.modData()), root_->getType()); + false, + root_->getHash(), + ledgerSeq_.load(std::memory_order_relaxed), + std::move(s.modData()), + root_->getType()); } return SHAMapAddNode::useful(); @@ -616,7 +620,11 @@ SHAMap::addKnownNode(SHAMapNodeID const& node, Slice const& rawNode, SHAMapSyncF Serializer s; newNode->serializeWithPrefix(s); filter->gotNode( - false, childHash, ledgerSeq_, std::move(s.modData()), newNode->getType()); + false, + childHash, + ledgerSeq_.load(std::memory_order_relaxed), + std::move(s.modData()), + newNode->getType()); } return SHAMapAddNode::useful(); diff --git a/src/xrpld/app/main/Application.cpp b/src/xrpld/app/main/Application.cpp index 57b0a575d8..3514c32193 100644 --- a/src/xrpld/app/main/Application.cpp +++ b/src/xrpld/app/main/Application.cpp @@ -1115,12 +1115,10 @@ private: bool ApplicationImp::setup(boost::program_options::variables_map const& cmdline) { - // NOTE: IO threads are intentionally NOT started here. - // All setup work is non-blocking (it enqueues async work to the - // io_context but doesn't require it to be processed yet). - // IO threads are started at the beginning of start() to avoid - // data races between the main thread doing setup and IO threads - // processing enqueued work concurrently. + // NOTE: The main io_context threads are NOT started until start(). + // However, subsystem-specific threads (resource manager, nodestore + // read threads) are started here because they use their own + // threading and do not contend with io_context setup work. m_resourceManager->start(); m_nodeStore->startReadThreads(); diff --git a/src/xrpld/app/main/BasicApp.cpp b/src/xrpld/app/main/BasicApp.cpp index 4ae5c04771..8a16b593c6 100644 --- a/src/xrpld/app/main/BasicApp.cpp +++ b/src/xrpld/app/main/BasicApp.cpp @@ -20,9 +20,9 @@ BasicApp::startIOThreads() { threads_.reserve(numberOfThreads_); - while (numberOfThreads_--) + for (std::size_t i = 0; i < numberOfThreads_; ++i) { - threads_.emplace_back([this, n = numberOfThreads_]() { + threads_.emplace_back([this, n = i]() { beast::setCurrentThreadName("io svc #" + std::to_string(n)); this->io_context_.run(); });