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
(cherry picked from commit 943d63e45ad09f75daa7d2f3dcc17bb28fda1766)

2 files changed, 73 insertions, 14 deletions

diff --git a/src/corelib/tools/qsharedpointer_impl.h b/src/corelib/tools/qsharedpointer_impl.h
index 10331c8..593da8f 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 cec31c2..94b7ce5 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"