From f9b2d725a00328bab6a3f2f625ccd02260554a72 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 11:26:29 +0100 Subject: [PATCH 1/3] fix(telemetry): assert SpanGuard Scope is destroyed on its constructing thread Co-Authored-By: Claude Opus 4.8 (1M context) --- include/xrpl/telemetry/SpanGuard.h | 7 ++++++- src/libxrpl/telemetry/SpanGuard.cpp | 25 +++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index ea8a121a0e..88814bfcb5 100644 --- a/include/xrpl/telemetry/SpanGuard.h +++ b/include/xrpl/telemetry/SpanGuard.h @@ -149,6 +149,10 @@ * it may be moved to and destroyed on another thread; detached() * itself must be called on the origin (constructing) thread. Use * captureContext() to propagate the trace context to other threads. + * Violating this rule is enforced (not just documented): a scoped + * guard destroyed on a foreign thread, or detached() called from one, + * trips an XRPL_ASSERT in debug/test/fuzzing builds instead of + * silently corrupting the other thread's context stack. * * @note Move semantics: Move construction transfers ownership of * the pimpl pointer — no double-Scope issues. Move assignment is @@ -333,7 +337,8 @@ public: * * @return A scope-less guard safe to move to and destroy on * another thread, or a null guard if this guard was null. - * @note Must be called on the origin (constructing) thread. The + * @note Must be called on the origin (constructing) thread; this is + * checked by an XRPL_ASSERT in debug/test/fuzzing builds. The * returned guard may be freely moved across threads; only * its final destruction ends the span. */ diff --git a/src/libxrpl/telemetry/SpanGuard.cpp b/src/libxrpl/telemetry/SpanGuard.cpp index b0437b8e17..3b920e51b3 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -25,6 +25,7 @@ #include +#include #include #include @@ -44,6 +45,7 @@ #include #include #include +#include #include #include @@ -89,6 +91,16 @@ struct SpanGuard::Impl */ std::optional scope; + /** + * Thread that constructed this Impl. Meaningful only when `scope` + * holds a value: a Scope is bound to the thread-local context stack + * of the thread that pushed it, so popping it (via ~Scope, when + * ~Impl runs) on a different thread corrupts that thread's stack. + * Checked in ~Impl() and detached() to turn a silent cross-thread + * corruption into an assertion failure in debug/test/fuzzing builds. + */ + std::thread::id owner{std::this_thread::get_id()}; + /** * Construct a scoped guard: the span is pushed onto this thread's * active-context stack for the lifetime of the guard. @@ -118,6 +130,14 @@ struct SpanGuard::Impl ~Impl() { + // A live Scope (scope engaged) must be popped on the thread that + // pushed it; ending on any other thread corrupts that thread's + // context stack. A detached guard (scope == nullopt) carries no + // binding and may be destroyed anywhere, so the check is skipped. + XRPL_ASSERT( + !scope.has_value() || owner == std::this_thread::get_id(), + "xrpl::telemetry::SpanGuard::Impl::~Impl : scoped guard destroyed on " + "constructing thread"); if (span) span->End(); } @@ -312,6 +332,11 @@ SpanGuard::detached() && { if (!impl_) return {}; + // Popping the Scope (below, via impl_.reset()) must happen on the thread + // that pushed it; calling detached() elsewhere would pop the wrong stack. + XRPL_ASSERT( + !impl_->scope.has_value() || impl_->owner == std::this_thread::get_id(), + "xrpl::telemetry::SpanGuard::detached : called on constructing thread"); // Take the span out; the old Impl.span is now null so ~Impl won't End(). auto s = std::move(impl_->span); // Resetting the old Impl destroys its Scope HERE, on the origin thread, From f59682fb7507aa02c127958195c64b5f987f1a81 Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 11:30:47 +0100 Subject: [PATCH 2/3] test(telemetry): convert cross-thread scope-leak negative control to a death test Co-Authored-By: Claude Opus 4.8 (1M context) --- .../libxrpl/telemetry/SpanGuardScope.cpp | 61 ++++++++++--------- 1 file changed, 33 insertions(+), 28 deletions(-) diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index a06d114dfd..54612057bf 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -381,38 +381,43 @@ TEST_F(SpanGuardScopeTest, root_then_detached_is_root_and_leaves_stack_clean) EXPECT_EQ(child->GetTraceId(), ambient->GetTraceId()); } -// Negative control (documents the bug detached() fixes): a scoped guard NOT -// detached, moved to and destroyed on a worker thread, pops the WRONG thread's -// stack, so a later span on the origin thread inherits the stale span. The -// whole scenario runs on a dedicated origin thread so the corrupted -// thread-local stack cannot leak into other tests. -TEST_F(SpanGuardScopeTest, non_detached_cross_thread_corrupts_origin_stack) +// Death test guarding the cross-thread scope-leak bug. +// +// This used to be a "negative control": a scoped guard was NOT detached, moved +// to and destroyed on a worker thread (popping the WRONG thread's stack), and +// the test then asserted that a later span on the origin thread had SILENTLY +// inherited the stale span -- proving the corruption happened undetected. +// +// SpanGuard::Impl::~Impl now enforces thread affinity with an XRPL_ASSERT: a +// live scope must be destroyed on the thread that created it. In debug/test +// builds (NDEBUG unset) XRPL_ASSERT expands to assert(), so the exact sequence +// above now aborts the process inside the worker thread's destructor instead of +// corrupting the origin stack. The bug therefore moved from "silently wrong" to +// "loudly crashes early" -- the intended tradeoff, since a crash is far easier +// to catch than a corrupted trace hierarchy. +// +// The death happens on the WORKER thread (the moved-in guard is destroyed when +// the worker lambda returns, before worker.join() completes). A failed assert() +// calls abort(), which raises SIGABRT process-wide regardless of which thread +// hit it, so EXPECT_DEATH -- which runs the statement in a forked child and +// checks it dies -- observes the crash. The regex matches the assert message +// substring rather than the whole line, because the file:line/function prefix +// glibc prints around it is platform and compiler dependent. +TEST_F(SpanGuardScopeTest, non_detached_cross_thread_death_asserts_at_wrong_thread_destroy) { - std::thread origin([&] { - auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); + EXPECT_DEATH( { - // ~SpanGuard on the worker ends the span and Detaches on the - // WORKER stack -- the origin stack is never popped. + // Scoped guard is active on this (origin) thread; its Scope is + // bound to this thread. + auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); + + // Move the still-scoped guard to a worker thread WITHOUT detaching. + // ~SpanGuard::Impl runs on the worker when the lambda returns and + // trips the thread-affinity assertion -> abort(). std::thread worker([d = std::move(guard)]() mutable {}); worker.join(); - } - // Origin stack still holds the stale ledger.build as active, so this - // span inherits it as parent instead of being a root. - auto after = SpanGuard::span(TraceCategory::Rpc, "rpc", "command"); - ASSERT_TRUE(static_cast(after)); - }); - origin.join(); - - auto spans = spanData()->GetSpans(); - auto* build = findSpan(spans, "ledger.build"); - auto* after = findSpan(spans, "rpc.command"); - ASSERT_NE(build, nullptr); - ASSERT_NE(after, nullptr); - - // BUG: 'after' inherited the stale span -- non-zero parent, same trace. - EXPECT_TRUE(after->GetParentSpanId().IsValid()); - EXPECT_EQ(after->GetParentSpanId(), build->GetSpanId()); - EXPECT_EQ(after->GetTraceId(), build->GetTraceId()); + }, + ".*scoped guard destroyed on constructing thread.*"); } // detachInPlace(optional&) must detach the live guard the same way the From 8d8bc8b92886245f403447ad4d24c6625a97022a Mon Sep 17 00:00:00 2001 From: Pratik Mankawde <3397372+pratikmankawde@users.noreply.github.com> Date: Tue, 21 Jul 2026 11:41:29 +0100 Subject: [PATCH 3/3] fix(telemetry): widen death-test regex for split assert literal, skip under NDEBUG Co-Authored-By: Claude Opus 4.8 (1M context) --- .../libxrpl/telemetry/SpanGuardScope.cpp | 27 ++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index 54612057bf..9295086a3a 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -403,8 +403,28 @@ TEST_F(SpanGuardScopeTest, root_then_detached_is_root_and_leaves_stack_clean) // checks it dies -- observes the crash. The regex matches the assert message // substring rather than the whole line, because the file:line/function prefix // glibc prints around it is platform and compiler dependent. +// +// The test is skipped where the assertion cannot fire: under NDEBUG (Release +// builds) XRPL_ASSERT is a no-op, and under ENABLE_VOIDSTAR a failed assert +// continues instead of aborting -- in both cases the worker would not crash and +// EXPECT_DEATH would report a spurious failure. TEST_F(SpanGuardScopeTest, non_detached_cross_thread_death_asserts_at_wrong_thread_destroy) { + // XRPL_ASSERT only aborts when assertions are live. Under NDEBUG (Release + // builds) it expands to assert(), which the C standard strips to a no-op, + // so the worker thread destroys the guard without crashing. Under + // ENABLE_VOIDSTAR (Antithesis fuzzing) a failed XRPL_ASSERT continues + // execution instead of aborting. In either case the process does not die + // and EXPECT_DEATH would fail, so skip this test there. The two macros are + // mutually exclusive (instrumentation.h #errors on ENABLE_VOIDSTAR+NDEBUG), + // but both break the death check, so guard against each. +#ifdef NDEBUG + GTEST_SKIP() << "XRPL_ASSERT compiles to a no-op under NDEBUG (Release builds), so the " + "cross-thread scope-leak assertion this test exercises does not fire."; +#elif defined(ENABLE_VOIDSTAR) + GTEST_SKIP() << "ENABLE_VOIDSTAR continues past a failed XRPL_ASSERT instead of aborting, so " + "the cross-thread scope-leak assertion this test exercises does not crash."; +#else EXPECT_DEATH( { // Scoped guard is active on this (origin) thread; its Scope is @@ -417,7 +437,12 @@ TEST_F(SpanGuardScopeTest, non_detached_cross_thread_death_asserts_at_wrong_thre std::thread worker([d = std::move(guard)]() mutable {}); worker.join(); }, - ".*scoped guard destroyed on constructing thread.*"); + // The assert message is written as two adjacent string literals in + // SpanGuard.cpp; glibc's assert() stringifies the expression source via + // the preprocessor '#' operator, keeping both quoted literals with the + // "\" \"" gap between "on" and "constructing". Match across that gap. + ".*scoped guard destroyed on.*constructing thread.*"); +#endif } // detachInPlace(optional&) must detach the live guard the same way the