Endpoint factory manager destroy race condition - Fix zeroc-ice#2585

pepone · Sep 25, 2024 · 16ecd7a · 16ecd7a
1 parent 5e920e8
commit 16ecd7a
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 7 deletions.
diff --git a/cpp/src/Ice/EndpointFactoryManager.cpp b/cpp/src/Ice/EndpointFactoryManager.cpp
@@ -31,7 +31,11 @@ IceInternal::EndpointFactoryManager::initialize() const
 void
 IceInternal::EndpointFactoryManager::add(const EndpointFactoryPtr& factory)
 {
-    lock_guard lock(_mutex); // TODO: Necessary?
+    lock_guard lock(_mutex);
+    if (_destroyed)
+    {
+        throw CommunicatorDestroyedException(__FILE__, __LINE__);
+    }
 
     //
     // TODO: Optimize with a map?
@@ -49,7 +53,11 @@ IceInternal::EndpointFactoryManager::add(const EndpointFactoryPtr& factory)
 EndpointFactoryPtr
 IceInternal::EndpointFactoryManager::get(int16_t type) const
 {
-    lock_guard lock(_mutex); // TODO: Necessary?
+    lock_guard lock(_mutex);
+    if (_destroyed)
+    {
+        throw CommunicatorDestroyedException(__FILE__, __LINE__);
+    }
 
     //
     // TODO: Optimize with a map?
@@ -89,8 +97,12 @@ IceInternal::EndpointFactoryManager::create(const string& str, bool oaEndpoint)
 
     EndpointFactoryPtr factory;
     {
-        lock_guard lock(_mutex); // TODO: Necessary?
-
+        // Hold the mutex while looking up the factory, destroy can be called concurrently.
+        lock_guard lock(_mutex);
+        if (_destroyed)
+        {
+            throw CommunicatorDestroyedException(__FILE__, __LINE__);
+        }
         //
         // TODO: Optimize with a map?
         //
@@ -189,6 +201,8 @@ IceInternal::EndpointFactoryManager::read(InputStream* s) const
 void
 IceInternal::EndpointFactoryManager::destroy()
 {
+    lock_guard lock(_mutex);
+    _destroyed = true;
     for (vector<EndpointFactoryPtr>::size_type i = 0; i < _factories.size(); i++)
     {
         _factories[i]->destroy();

diff --git a/cpp/src/Ice/EndpointFactoryManager.h b/cpp/src/Ice/EndpointFactoryManager.h
@@ -38,6 +38,7 @@ namespace IceInternal
         InstancePtr _instance;
         std::vector<EndpointFactoryPtr> _factories;
         mutable std::mutex _mutex;
+        bool _destroyed = false;
     };
 }
 

diff --git a/csharp/src/Ice/Internal/EndpointFactoryManager.cs b/csharp/src/Ice/Internal/EndpointFactoryManager.cs
@@ -24,6 +24,11 @@ public void add(EndpointFactory factory)
     {
         lock (_mutex)
         {
+            if (_destroyed)
+            {
+                throw new Ice.CommunicatorDestroyedException();
+            }
+
             foreach (EndpointFactory f in _factories)
             {
                 if (f.type() == factory.type())
@@ -39,6 +44,11 @@ public EndpointFactory get(short type)
     {
         lock (_mutex)
         {
+            if (_destroyed)
+            {
+                throw new Ice.CommunicatorDestroyedException();
+            }
+
             foreach (EndpointFactory f in _factories)
             {
                 if (f.type() == type)
@@ -76,6 +86,10 @@ public EndpointI create(string str, bool oaEndpoint)
 
         lock (_mutex)
         {
+            if (_destroyed)
+            {
+                throw new Ice.CommunicatorDestroyedException();
+            }
             for (int i = 0; i < _factories.Count; i++)
             {
                 EndpointFactory f = _factories[i];
@@ -182,13 +196,18 @@ public EndpointI read(Ice.InputStream s)
 
     internal void destroy()
     {
-        foreach (EndpointFactory f in _factories)
+        lock (_mutex)
         {
-            f.destroy();
+            _destroyed = true;
+            foreach (EndpointFactory f in _factories)
+            {
+                f.destroy();
+            }
+            _factories.Clear();
         }
-        _factories.Clear();
     }
 
+    private bool _destroyed;
     private readonly Instance _instance;
     private readonly List<EndpointFactory> _factories;
     private readonly object _mutex = new();

diff --git a/java/src/Glacier2/src/main/java/module-info.java b/java/src/Glacier2/src/main/java/module-info.java
@@ -1,5 +1,6 @@
 // Copyright (c) ZeroC, Inc.
 
+@SuppressWarnings("module")
 module com.zeroc.glacier2 {
     exports com.zeroc.Glacier2;
     exports com.zeroc.Glacier2.IceMX;

diff --git a/java/src/Ice/src/main/java/com/zeroc/Ice/EndpointFactoryManager.java b/java/src/Ice/src/main/java/com/zeroc/Ice/EndpointFactoryManager.java
@@ -16,6 +16,10 @@ public void initialize() {
     }
 
     public synchronized void add(EndpointFactory factory) {
+        if (_destroyed) {
+            throw new CommunicatorDestroyedException();
+        }
+
         for (EndpointFactory f : _factories) {
             if (f.type() == factory.type()) {
                 assert (false);
@@ -25,6 +29,10 @@ public synchronized void add(EndpointFactory factory) {
     }
 
     public synchronized EndpointFactory get(short type) {
+        if (_destroyed) {
+            throw new CommunicatorDestroyedException();
+        }
+
         for (EndpointFactory f : _factories) {
             if (f.type() == type) {
                 return f;
@@ -34,6 +42,11 @@ public synchronized EndpointFactory get(short type) {
     }
 
     public synchronized EndpointI create(String str, boolean oaEndpoint) {
+
+        if (_destroyed) {
+            throw new CommunicatorDestroyedException();
+        }
+
         String[] arr = StringUtil.splitString(str, " \t\r\n");
         if (arr == null) {
             throw new ParseException("Failed to parse endpoint '" + str + "': mismatched quote");
@@ -163,6 +176,7 @@ void destroy() {
         _factories.clear();
     }
 
+    private boolean _destroyed = false;
     private Instance _instance;
     private java.util.List<EndpointFactory> _factories = new java.util.ArrayList<>();
 }