From ddbc5f1a2411c597b7be3038011cac855490a87f Mon Sep 17 00:00:00 2001 From: Alex Kremer Date: Wed, 30 Sep 2026 15:51:25 +0000 Subject: [PATCH] fix: Resolve IntrusivePointer leak (#8328) Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com> --- include/xrpl/basics/IntrusivePointer.ipp | 13 +-- include/xrpl/basics/IntrusiveRefCounts.h | 52 +++++------- src/tests/libxrpl/basics/IntrusiveShared.cpp | 89 ++++++++++++++++++++ 3 files changed, 118 insertions(+), 36 deletions(-) diff --git a/include/xrpl/basics/IntrusivePointer.ipp b/include/xrpl/basics/IntrusivePointer.ipp index 6c2a71f7eb..2417efdb28 100644 --- a/include/xrpl/basics/IntrusivePointer.ipp +++ b/include/xrpl/basics/IntrusivePointer.ipp @@ -618,13 +618,16 @@ SharedWeakUnion::convertToWeak() unsafeSetRawPtr(nullptr); return true; // Should never happen // LCOV_EXCL_STOP - case PartialDestroy: - // This is a weird case. We just converted the last strong - // pointer to a weak pointer. + case PartialDestroy: { + // We just converted the last strong pointer to a weak pointer. + // The weak ref we now hold keeps the object from being fully + // destroyed, so `p` stays valid; only the copy passed to + // `partialDestructorFinished` is nulled. p->partialDestructor(); - partialDestructorFinished(&p); - // p is null and may no longer be used + auto finished = p; + partialDestructorFinished(&finished); break; + } } unsafeSetRawPtr(p, RefStrength::Weak); return true; diff --git a/include/xrpl/basics/IntrusiveRefCounts.h b/include/xrpl/basics/IntrusiveRefCounts.h index caa06ed786..7aa3d33aee 100644 --- a/include/xrpl/basics/IntrusiveRefCounts.h +++ b/include/xrpl/basics/IntrusiveRefCounts.h @@ -77,21 +77,13 @@ struct IntrusiveRefCounts std::size_t useCount() const noexcept; - // This function MUST be called after a partial destructor finishes running. - // Calling this function may cause other threads to delete the object - // pointed to by `o`, so `o` should never be used after calling this - // function. The parameter will be set to a `nullptr` after calling this - // function to emphasize that it should not be used. - // Note: This is intentionally NOT called at the end of `partialDestructor`. - // The reason for this is if new classes are written to support this smart - // pointer class, they need to write their own `partialDestructor` function - // and ensure `partialDestructorFinished` is called at the end. Putting this - // call inside the smart pointer class itself is expected to be less error - // prone. - // Note: The "two-star" programming is intentional. It emphasizes that `o` - // may be deleted and the unergonomic API is meant to signal the special - // nature of this function call to callers. - // Note: This is a template to support incompletely defined classes. + // MUST be called after `partialDestructor` returns. Another thread may + // then delete the object, so `*o` is nulled and must not be used after + // (unless the caller holds its own weak ref, e.g. + // SharedWeakUnion::convertToWeak). Called by the smart pointers, not + // `partialDestructor`, so custom partial destructors can't forget it. + // The two-star API signals that `*o` may be deleted. Templated to + // support incomplete types. template friend void partialDestructorFinished(T** o); @@ -334,15 +326,11 @@ IntrusiveRefCounts::addWeakReleaseStrongRef() const ReleaseStrongRefAction action = NoOp; if (prevVal.strong == 1) { - if (prevVal.weak == 0) - { - action = NoOp; - } - else - { - nextIntVal |= kPartialDestroyStartedMask; - action = PartialDestroy; - } + // The weak ref added here keeps the weak count non-zero, so + // releasing the last strong ref always starts a partial destroy, + // regardless of the previous weak count. + nextIntVal |= kPartialDestroyStartedMask; + action = PartialDestroy; } if (refCounts_.compare_exchange_weak(prevIntVal, nextIntVal, std::memory_order_acq_rel)) { @@ -358,24 +346,26 @@ IntrusiveRefCounts::addWeakReleaseStrongRef() const inline ReleaseWeakRefAction IntrusiveRefCounts::releaseWeakRef() const { - auto prevIntVal = refCounts_.fetch_sub(kWeakDelta, std::memory_order_acq_rel); - RefCountPair prev = prevIntVal; + auto const prevIntVal = refCounts_.fetch_sub(kWeakDelta, std::memory_order_acq_rel); + RefCountPair const prev = prevIntVal; if (prev.weak == 1 && prev.strong == 0) { + // `wait` blocks while the value equals its argument, so it must be + // given the value as it is after the decrement above. + auto curIntVal = prevIntVal - kWeakDelta; if (prev.partialDestroyStartedBit == 0u) { // This case should only be hit if the partialDestroyStartedBit is // set non-atomically (and even then very rarely). The code is kept // in case we need to set the flag non-atomically for perf reasons. - refCounts_.wait(prevIntVal, std::memory_order_acquire); - prevIntVal = refCounts_.load(std::memory_order_acquire); - prev = RefCountPair{prevIntVal}; + refCounts_.wait(curIntVal, std::memory_order_acquire); + curIntVal = refCounts_.load(std::memory_order_acquire); } - if (prev.partialDestroyFinishedBit == 0u) + if (RefCountPair{curIntVal}.partialDestroyFinishedBit == 0u) { // partial destroy MUST finish before running a full destroy (when // using weak pointers) - refCounts_.wait(prevIntVal - kWeakDelta, std::memory_order_acquire); + refCounts_.wait(curIntVal, std::memory_order_acquire); } return ReleaseWeakRefAction::Destroy; } diff --git a/src/tests/libxrpl/basics/IntrusiveShared.cpp b/src/tests/libxrpl/basics/IntrusiveShared.cpp index c6c9fcfef0..e2965e2bec 100644 --- a/src/tests/libxrpl/basics/IntrusiveShared.cpp +++ b/src/tests/libxrpl/basics/IntrusiveShared.cpp @@ -436,6 +436,95 @@ TEST(IntrusiveSharedTest, partial_delete) EXPECT_TRUE(destructorRan.load() && partialDeleteRan.load()); } +TEST(IntrusiveSharedTest, convert_last_strong_to_weak) +{ + using enum TrackedState; + + TIBase::ResetStatesGuard const rsg{true}; + + SharedWeakUnion p = makeSharedIntrusive(); + auto const id = p.get()->id; + EXPECT_EQ(TIBase::getState(id), Alive); + + EXPECT_TRUE(p.convertToWeak()); + EXPECT_TRUE(p.isWeak()); + EXPECT_TRUE(p.expired()); + EXPECT_EQ(TIBase::getState(id), PartiallyDeleted); + + p.reset(); + EXPECT_EQ(TIBase::getState(id), Deleted); +} + +TEST(IntrusiveSharedTest, multithreaded_convert_to_weak_partial_delete) +{ + using enum TrackedState; + + TIBase::ResetStatesGuard const rsg{true}; + + SharedWeakUnion converted = makeSharedIntrusive(); + SharedWeakUnion other = converted; + EXPECT_TRUE(other.convertToWeak()); + EXPECT_EQ(converted.useCount(), 1); + auto const id = converted.get()->id; + + std::atomic destructorRan{false}; + std::atomic partialDeleteRan{false}; + std::latch partialDeleteStartedSyncPoint{1}; + std::latch otherReleasedSyncPoint{1}; + + TIBase::tracingCallback = [&](TrackedState cur, std::optional next) { + if (!next) + return; + + switch (*next) + { + case DeletedStarted: + EXPECT_EQ(cur, PartiallyDeleted); + break; + + case PartiallyDeletedStarted: + partialDeleteStartedSyncPoint.count_down(); + // Keep the partial delete running until the other weak pointer has been released. + otherReleasedSyncPoint.wait(); + break; + + case PartiallyDeleted: + EXPECT_FALSE(partialDeleteRan.exchange(true) || destructorRan.load()); + break; + + case Deleted: + EXPECT_FALSE(destructorRan.exchange(true)); + break; + + case Uninitialized: + case Alive: + break; + } + }; + + std::thread t1{[&] { + partialDeleteStartedSyncPoint.wait(); + other.reset(); // Not the last weak ref, so must not trigger a delete + otherReleasedSyncPoint.count_down(); + }}; + + std::thread t2{[&] { + EXPECT_TRUE(converted.convertToWeak()); // Trigger a partial delete + }}; + + t1.join(); + t2.join(); + + EXPECT_TRUE(partialDeleteRan.load()); + EXPECT_FALSE(destructorRan.load()); + EXPECT_TRUE(converted.isWeak()); + EXPECT_EQ(TIBase::getState(id), PartiallyDeleted); + + converted.reset(); // Release the last weak ref + EXPECT_TRUE(destructorRan.load()); + EXPECT_EQ(TIBase::getState(id), Deleted); +} + TEST(IntrusiveSharedTest, destructor) { // This test creates two threads. One with a strong pointer and one