From f2d429b81745e475609f1e981c1dac5ec513acc3 Mon Sep 17 00:00:00 2001
From: rdb <git@rdb.name>
Date: Sun, 27 May 2018 13:06:57 +0200
Subject: [PATCH] express: weakptr.lock() should return null during object
 destruction

This came up in #330; the Character destructor caused something to call lock() on a weak pointer to that character, which would induce a ref() and unref() pair, but since the refcount was 0, this would call the destructor and thereby create infinite recursion.

I considered instead calling mark_deleted() inside unref() so that the callbacks get to run before the object is actually deleted, and was_deleted() will become true as soon as unref() reaches 0.  However, this would require grabbing the lock in unref() to be fully thread-safe, since we would need to bring the refcount to 0 and mark the object as deleted in one atomic operation, so this would be an unacceptable general performance penalty.

Instead, WeakPointerTo::lock() now atomically increments the reference count if it is not already zero, and returns null otherwise.  This should be safe because the object cannot be deleted while the WeakReferenceList lock is held.
---
 panda/src/express/referenceCount.I | 21 +++++++++++++++++++++
 panda/src/express/referenceCount.h |  2 ++
 panda/src/express/weakPointerTo.I  | 19 +++++++++++++++++--
 3 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/panda/src/express/referenceCount.I b/panda/src/express/referenceCount.I
index 3a05a68766..f18c60a02c 100644
--- a/panda/src/express/referenceCount.I
+++ b/panda/src/express/referenceCount.I
@@ -300,6 +300,27 @@ weak_unref() {
   nassertv(nonzero);
 }
 
+/**
+ * Atomically increases the reference count of this object if it is not zero.
+ * Do not use this.  This exists only to implement a special case for weak
+ * pointers.
+ * @return true if the reference count was incremented, false if it was zero.
+ */
+INLINE bool ReferenceCount::
+ref_if_nonzero() const {
+#ifdef _DEBUG
+  test_ref_count_integrity();
+#endif
+  AtomicAdjust::Integer ref_count;
+  do {
+    ref_count = AtomicAdjust::get(_ref_count);
+    if (ref_count <= 0) {
+      return false;
+    }
+  } while (ref_count != AtomicAdjust::compare_and_exchange(_ref_count, ref_count, ref_count + 1));
+  return true;
+}
+
 /**
  * This global helper function will unref the given ReferenceCount object, and
  * if the reference count reaches zero, automatically delete it.  It can't be
diff --git a/panda/src/express/referenceCount.h b/panda/src/express/referenceCount.h
index 2260cf2e8c..ba27ac7db8 100644
--- a/panda/src/express/referenceCount.h
+++ b/panda/src/express/referenceCount.h
@@ -63,6 +63,8 @@ public:
   INLINE WeakReferenceList *weak_ref();
   INLINE void weak_unref();
 
+  INLINE bool ref_if_nonzero() const;
+
 protected:
   bool do_test_ref_count_integrity() const;
   bool do_test_ref_count_nonzero() const;
diff --git a/panda/src/express/weakPointerTo.I b/panda/src/express/weakPointerTo.I
index 09dba7640b..60ac8a1917 100644
--- a/panda/src/express/weakPointerTo.I
+++ b/panda/src/express/weakPointerTo.I
@@ -75,6 +75,9 @@ operator T * () const {
 /**
  * A thread-safe way to access the underlying pointer; will silently return
  * null if the underlying pointer was deleted or null.
+ * Note that this may return null even if was_deleted() still returns true,
+ * which can occur if the object has reached reference count 0 and is about to
+ * be destroyed.
  */
 template<class T>
 INLINE PointerTo<T> WeakPointerTo<T>::
@@ -84,7 +87,13 @@ lock() const {
     PointerTo<T> ptr;
     weak_ref->_lock.lock();
     if (!weak_ref->was_deleted()) {
-      ptr = (To *)WeakPointerToBase<T>::_void_ptr;
+      // We also need to check that the reference count is not zero (which can
+      // happen if the object is currently being destructed), since that could
+      // cause double deletion.
+      To *plain_ptr = (To *)WeakPointerToBase<T>::_void_ptr;
+      if (plain_ptr != nullptr && plain_ptr->ref_if_nonzero()) {
+        ptr.cheat() = plain_ptr;
+      }
     }
     weak_ref->_lock.unlock();
     return ptr;
@@ -239,7 +248,13 @@ lock() const {
     ConstPointerTo<T> ptr;
     weak_ref->_lock.lock();
     if (!weak_ref->was_deleted()) {
-      ptr = (const To *)WeakPointerToBase<T>::_void_ptr;
+      // We also need to check that the reference count is not zero (which can
+      // happen if the object is currently being destructed), since that could
+      // cause double deletion.
+      const To *plain_ptr = (const To *)WeakPointerToBase<T>::_void_ptr;
+      if (plain_ptr != nullptr && plain_ptr->ref_if_nonzero()) {
+        ptr.cheat() = plain_ptr;
+      }
     }
     weak_ref->_lock.unlock();
     return ptr;