diff --git a/include/xrpl/telemetry/SpanGuard.h b/include/xrpl/telemetry/SpanGuard.h index f011f90f76..11ace20a49 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 @@ -364,7 +368,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 b4bf489d8e..f9baac4df8 100644 --- a/src/libxrpl/telemetry/SpanGuard.cpp +++ b/src/libxrpl/telemetry/SpanGuard.cpp @@ -26,6 +26,7 @@ #include #include +#include #include #include #include @@ -54,6 +55,7 @@ #include #include #include +#include #include #include #include @@ -100,6 +102,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. @@ -129,6 +141,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(); } @@ -327,6 +347,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, diff --git a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp index e6bbf4cec8..21a69841a9 100644 --- a/src/tests/libxrpl/telemetry/SpanGuardScope.cpp +++ b/src/tests/libxrpl/telemetry/SpanGuardScope.cpp @@ -392,38 +392,68 @@ 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. +// +// 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) { - std::thread origin([&] { - auto guard = SpanGuard::span(TraceCategory::Ledger, "ledger", "build"); + // 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( { - // ~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()); + }, + // 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