CBL-3624: double free of BLIPIO (#1575)

The cause of this bug is in WeakHolder -- it currently works only if the callback it invokes is synchronous. We fix it by holding an extra strong reference inside WeakHolder.

LiteCore/Support/WeakHolder.hh

@@ -12,29 +12,26 @@
 
 #pragma once
 #include "fleece/RefCounted.hh"
-#include <shared_mutex>
 
 namespace litecore {
 
 /** WeakHolder<T>: holds a pointer to T weakly. Unlike general weak reference, one cannot get a strong holder from it.
-    Instead, we can call the methods of class T via invoke, which returns true if the call goes through as the underlying
-    pointer is good. */
+    Instead, we can call the methods of class T via invoke, which returns true if pointer is strongly referenced by some
+    object other than *this.
+    Pre-conditions: T must be dynamically castable to RefCounted.
+    Note: WeakHolder holds a strong reference but provides no API to release it. Threfore, leaking of
+    WeakHolder also leaks the object being held. It also implies that you cannot rely on the destructor of the held object
+    to auto-release the WeakHolder.
+ */
 template <typename T>
 class WeakHolder : public RefCounted {
 public:
     WeakHolder(T* pointer)
     : _pointer(pointer)
     {
         DebugAssert(_pointer != nullptr);
-    }
-
-    // Only the original owner may rescind the pointer.
-    // After rescind(), the held pointer becomes nullptr and invoke() returns false.
-    void rescind(T* owner) {
-        if (owner == _pointer) {
-            std::lock_guard<std::shared_mutex> lock(_mutex);
-            _pointer = nullptr;
-        }
+        _holder = dynamic_cast<RefCounted*>(_pointer);
+        Assert(_holder);
     }
 
     /** Call the member function with the underlying pointer.
@@ -44,17 +41,19 @@ public:
         @warning what is returned from the member fundtion, if not void, will be thrown away. */
     template<typename MemFuncPtr, typename ... Args>
     bool invoke(MemFuncPtr memFuncPtr, Args&& ... args) {
-        std::shared_lock<std::shared_mutex> shl(_mutex);
-        if (_pointer == nullptr) {
+        Retained<RefCounted> holdingIt = _holder;
+        if (_holder->refCount() == 2) {
+            // There is no place outside here do references exist.
             return false;
         }
         (_pointer->*memFuncPtr)(std::forward<Args>(args)...);
         return true;
     }
 
 private:
+    // Invariant: dynamic_cast<RefCounted*>(_pointer) == _holder.get()
     T*    _pointer;
-    std::shared_mutex _mutex;
+    Retained<RefCounted> _holder;
 };
 
 }

Networking/BLIP/BLIPConnection.cc

@@ -137,6 +137,7 @@ namespace litecore { namespace blip {
                 _webSocket->close();
                 _webSocket = nullptr;
                 _connection = nullptr;
+                _weakThis = nullptr;
             }
         }
 
@@ -171,7 +172,6 @@ namespace litecore { namespace blip {
                   _timeOpen.elapsed(),
                   _maxOutboxDepth, _totalOutboxDepth/(double)_countOutboxDepth);
             logStats();
-            _weakThis->rescind(this);
         }
 
         virtual void onWebSocketGotHTTPResponse(int status,
@@ -249,6 +249,7 @@ namespace litecore { namespace blip {
                 cancelAll(_pendingRequests);
                 cancelAll(_pendingResponses);
                 _requestHandlers.clear();
+                _weakThis = nullptr;
                 release(this); // webSocket is done calling delegate now (balances retain in ctor)
             } else {
                 warn("_closed called on a null connection");