fix: Resolve IntrusivePointer leak (#8328)

Co-authored-by: Valentin Balaschenko <13349202+vlntb@users.noreply.github.com>

include/xrpl/basics/IntrusivePointer.ipp

@@ -618,13 +618,16 @@ SharedWeakUnion<T>::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;

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 <class T>
     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;
     }

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<TIBase> p = makeSharedIntrusive<TIBase>();
+    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<TIBase> converted = makeSharedIntrusive<TIBase>();
+    SharedWeakUnion<TIBase> other = converted;
+    EXPECT_TRUE(other.convertToWeak());
+    EXPECT_EQ(converted.useCount(), 1);
+    auto const id = converted.get()->id;
+
+    std::atomic<bool> destructorRan{false};
+    std::atomic<bool> partialDeleteRan{false};
+    std::latch partialDeleteStartedSyncPoint{1};
+    std::latch otherReleasedSyncPoint{1};
+
+    TIBase::tracingCallback = [&](TrackedState cur, std::optional<TrackedState> 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