Fix a crash when recursing into QSharedPointer from QSharedPointer::clear()

We used to delete the tracked object before the new tracking was
properly set up in QSharedPointer. That means if the tracked object's
destructor or deletion function recursed into the QSharedPointer, it
would find itself in an inconsistent state.

So instead finish the setup and only then call out to user code. That
ensures the internal state is always valid.

Task-number: QTBUG-11730
Reviewed-by: ossi

2 files changed, 73 insertions, 14 deletions

diff --git a/src/corelib/tools/qsharedpointer_impl.h b/src/corelib/tools/qsharedpointer_impl.h
index 964b279..d581751 100644
--- a/src/corelib/tools/qsharedpointer_impl.h
+++ b/src/corelib/tools/qsharedpointer_impl.h
@@ -323,12 +323,17 @@ namespace QtSharedPointer {
         typedef ExternalRefCountData Data;
 
         inline void ref() const { d->weakref.ref(); d->strongref.ref(); }
-        inline bool deref()
+        inline void deref()
+        { deref(d, this->value); }
+        static inline void deref(Data *d, T *value)
         {
+            if (!d) return;
             if (!d->strongref.deref()) {
-                internalDestroy();
+                if (!d->destroy())
+                    delete value;
             }
-            return d->weakref.deref();
+            if (!d->weakref.deref())
+                delete d;
         }
 
         inline void internalConstruct(T *ptr)
@@ -377,7 +382,7 @@ namespace QtSharedPointer {
         template <class X>
         inline ExternalRefCount(const ExternalRefCount<X> &other) : Basic<T>(other.value), d(other.d)
         { if (d) ref(); }
-        inline ~ExternalRefCount() { if (d && !deref()) delete d; }
+        inline ~ExternalRefCount() { deref(); }
 
         template <class X>
         inline void internalCopy(const ExternalRefCount<X> &other)
@@ -385,12 +390,6 @@ namespace QtSharedPointer {
             internalSet(other.d, other.data());
         }
 
-        inline void internalDestroy()
-        {
-            if (!d->destroy())
-                delete this->value;
-        }
-
         inline void internalSwap(ExternalRefCount &other)
         {
             qSwap(d, other.d);
@@ -423,10 +422,14 @@ namespace QtSharedPointer {
                 else
                     o = 0;
             }
-            if (d && !deref())
-                delete d;
-            d = o;
-            this->value = d && d->strongref ? actual : 0;
+
+            qSwap(d, o);
+            qSwap(this->value, actual);
+            if (!d || d->strongref == 0)
+                this->value = 0;
+
+            // dereference saved data
+            deref(o, actual);
         }
 
         Data *d;
diff --git a/tests/auto/qsharedpointer/tst_qsharedpointer.cpp b/tests/auto/qsharedpointer/tst_qsharedpointer.cpp
index cb32c9a..4bbc9d8 100644
--- a/tests/auto/qsharedpointer/tst_qsharedpointer.cpp
+++ b/tests/auto/qsharedpointer/tst_qsharedpointer.cpp
@@ -92,6 +92,8 @@ private slots:
     void creating();
     void creatingQObject();
     void mixTrackingPointerCode();
+    void reentrancyWhileDestructing();
+
     void threadStressTest_data();
     void threadStressTest();
     void validConstructs();
@@ -1770,6 +1772,60 @@ void tst_QSharedPointer::invalidConstructs()
     }
 }
 
+namespace QTBUG11730 {
+    struct IB
+    {
+        virtual ~IB() {}
+    };
+
+    struct IA
+    {
+        virtual QSharedPointer<IB> getB() = 0;
+    };
+
+    struct B: public IB
+    {
+        IA *m_a;
+        B(IA *a_a) :m_a(a_a)
+        { }
+        ~B()
+        {
+            QSharedPointer<IB> b = m_a->getB();
+        }
+    };
+
+    struct A: public IA
+    {
+        QSharedPointer<IB> b;
+
+        virtual QSharedPointer<IB> getB()
+        {
+            return b;
+        }
+
+        A()
+        {
+            b = QSharedPointer<IB>(new B(this));
+        }
+
+        ~A()
+        {
+            b.clear();
+        }
+    };
+}
+
+void tst_QSharedPointer::reentrancyWhileDestructing()
+{
+    // this bug is about recursing back into QSharedPointer::clear()
+    // from inside it
+    // that is, the destructor of the object being deleted recurses
+    // into the same QSharedPointer object.
+    // First reported as QTBUG-11730
+    QTBUG11730::A obj;
+}
+
+
 QTEST_MAIN(tst_QSharedPointer)
 
 #include "tst_qsharedpointer.moc"