ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-04-29 15:37:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Address code review findings for TSAN/ASAN PR

Browse Source 
- 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 <noreply@anthropic.com>
This commit is contained in:
Pratik Mankawde
2026-03-19 12:51:51 +00:00
parent 9ec3f091f2
commit 9dd2ca1e4a
8 changed files with 70 additions and 58 deletions
Download Patch File Download Diff File Expand all files Collapse all files

28
.github/scripts/strategy-matrix/generate.py vendored
View File

@@ -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(
{

25
include/xrpl/basics/sanitizers.h
View File

@@ -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

15
include/xrpl/beast/test/yield_to.h
View File

@@ -4,6 +4,8 @@
#pragma once
#include <xrpl/basics/sanitizers.h>
#include <boost/asio/executor_work_guard.hpp>
#include <boost/asio/io_context.hpp>
#include <boost/asio/spawn.hpp>
@@ -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.

13
include/xrpl/core/Coro.ipp
View File

@@ -1,6 +1,7 @@
#pragma once
#include <xrpl/basics/ByteUtilities.h>
#include <xrpl/basics/sanitizers.h>
#include <cstddef>
@@ -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 <class F>
JobQueue::Coro::Coro(Coro_create_t, JobQueue& jq, JobType type, std::string const& name, F&& f)

21
src/libxrpl/shamap/SHAMap.cpp
View File

@@ -139,7 +139,8 @@ intr_ptr::SharedPtr<SHAMapTreeNode>
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<NodeObject> 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<NodeObject> const& object) {
auto node = finishFetch(hash, object);
cb(node, hash);
@@ -911,7 +918,11 @@ SHAMap::writeNode(NodeObjectType t, intr_ptr::SharedPtr<SHAMapTreeNode> 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;
}

12
src/libxrpl/shamap/SHAMapSync.cpp
View File

@@ -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();

10
src/xrpld/app/main/Application.cpp
View File

@@ -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();

4
src/xrpld/app/main/BasicApp.cpp
View File

@@ -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();
});