[Core] Close a refcounting gap causing a race condition in ProxyPool

risingPhil · Mar 2, 2022 · 2bac2bc · 2bac2bc
1 parent 0e62bb4
commit 2bac2bc
Showing 1 changed file with 17 additions and 11 deletions.
diff --git a/Source/core/Proxy.h b/Source/core/Proxy.h
@@ -119,9 +119,7 @@ namespace Core {
 
         virtual uint32_t Release() const
         {
-            uint32_t Result;
-
-            if ((Result = Core::InterlockedDecrement(m_RefCount)) == 0) {
+            if (InternalRelease() == 0) {
                 delete this;
 
                 return (Core::ERROR_DESTRUCTION_SUCCEEDED);
@@ -130,6 +128,11 @@ namespace Core {
             return (Core::ERROR_NONE);
         }
 
+        uint32_t InternalRelease() const
+        {
+            return (Core::InterlockedDecrement(m_RefCount));
+        }
+
         inline operator CONTEXT&()
         {
             return (*this);
@@ -1083,15 +1086,18 @@ namespace Core {
         }
         uint32_t Release() const override
         {
-            if (ProxyService<ELEMENT>::LastRef() == true) {
+            uint32_t result = Core::ERROR_NONE;
+            uint32_t newCount = ProxyService<ELEMENT>::InternalRelease();
+
+            if (newCount == 0) {
                 // This can only happen of the parent has unlinked us, other wise 
                 // the last release is always in the Unlink..
                 ASSERT(_parent == nullptr);
                 const_cast<ThisClass*>(this)->__Relinquish<CONTAINER, ELEMENT, EXPOSED>();
-            }
-            uint32_t result = ProxyService<ELEMENT>::Release();
+                delete this;
 
-            if ((result != Core::ERROR_DESTRUCTION_SUCCEEDED) && (ProxyService<ELEMENT>::LastRef() == true)) {
+                result = Core::ERROR_DESTRUCTION_SUCCEEDED;
+            } else if (newCount == 1) {
                 const_cast<ThisClass*>(this)->Notify();
 
                 result = Core::ERROR_DESTRUCTION_SUCCEEDED;
@@ -1103,17 +1109,17 @@ namespace Core {
         {
             ASSERT(_parent != nullptr);
 
-            if (ProxyService<ELEMENT>::LastRef() == false) {
-                // This can only happen if the parent has unlinked us, while we are still being used somewhere..
-                const_cast<ThisClass*>(this)->__Unlink<CONTAINER, ELEMENT, EXPOSED>();
-            }
+            // This can only happen if the parent has unlinked us, while we are still being used somewhere..
+            const_cast<ThisClass*>(this)->__Unlink<CONTAINER, ELEMENT, EXPOSED>();
+
 
             // By incrementing this refcount the last reference count is definitily not reached, so safe to remove the parent as we are sure
             // that it will not be used while we clear it... 
             ProxyService<ELEMENT>::AddRef();
             _parent = nullptr;
             ProxyService<ELEMENT>::Release();
             ProxyService<ELEMENT>::Release();
+
         }
 
     private: