Fixed ICE-5379, Object Adapter should reject null servant, in C++, Java, C# and JS

18 files changed, 179 insertions, 28 deletions

diff --git a/cpp/src/Freeze/BackgroundSaveEvictorI.cpp b/cpp/src/Freeze/BackgroundSaveEvictorI.cpp
index 52c3a64969f..1da527561e3 100644
--- a/cpp/src/Freeze/BackgroundSaveEvictorI.cpp
+++ b/cpp/src/Freeze/BackgroundSaveEvictorI.cpp
@@ -233,6 +233,7 @@ Ice::ObjectPrx
 Freeze::BackgroundSaveEvictorI::addFacet(const ObjectPtr& servant, const Identity& ident, const string& facet)
 {
     checkIdentity(ident);
+    checkServant(servant);
     DeactivateController::Guard deactivateGuard(_deactivateController);
    
     ObjectStore<BackgroundSaveEvictorElement>* store = findStore(facet, _createDb);
diff --git a/cpp/src/Freeze/EvictorI.cpp b/cpp/src/Freeze/EvictorI.cpp
index 626fbd47231..c9c4be90440 100644
--- a/cpp/src/Freeze/EvictorI.cpp
+++ b/cpp/src/Freeze/EvictorI.cpp
@@ -275,8 +275,6 @@ Freeze::EvictorIBase::locate(const Current& current, LocalObjectPtr& cookie)
 }
 
 
-
-
 void 
 Freeze::EvictorIBase::initialize(const Identity& ident, const string& facet, const ObjectPtr& servant)
 {
diff --git a/cpp/src/Freeze/EvictorI.h b/cpp/src/Freeze/EvictorI.h
index 8995b76da29..ec2792fe541 100644
--- a/cpp/src/Freeze/EvictorI.h
+++ b/cpp/src/Freeze/EvictorI.h
@@ -342,6 +342,15 @@ checkIdentity(const Ice::Identity& ident)
     }
 }
 
+inline void
+checkServant(const Ice::ObjectPtr& servant)
+{
+    if(servant == 0)
+    {
+        throw Ice::IllegalServantException(__FILE__, __LINE__, "cannot add null servant to Freeze Evictor");
+    }
+}
+
 }
 
 #endif
diff --git a/cpp/src/Freeze/TransactionalEvictorI.cpp b/cpp/src/Freeze/TransactionalEvictorI.cpp
index c41531f34ae..46680e0ee0f 100644
--- a/cpp/src/Freeze/TransactionalEvictorI.cpp
+++ b/cpp/src/Freeze/TransactionalEvictorI.cpp
@@ -137,6 +137,7 @@ ObjectPrx
 Freeze::TransactionalEvictorI::addFacet(const ObjectPtr& servant, const Identity& ident, const string& facet)
 {
     checkIdentity(ident);
+    checkServant(servant);
     DeactivateController::Guard deactivateGuard(_deactivateController);
    
     ObjectStore<TransactionalEvictorElement>* store = findStore(facet, _createDb);
diff --git a/cpp/src/Ice/Exception.cpp b/cpp/src/Ice/Exception.cpp
index 0fd53c3cae2..af4e44e6087 100644
--- a/cpp/src/Ice/Exception.cpp
+++ b/cpp/src/Ice/Exception.cpp
@@ -323,6 +323,14 @@ Ice::IllegalIdentityException::ice_print(ostream& out) const
     out << "'";
 }
 
+void
+Ice::IllegalServantException::ice_print(ostream& out) const
+{
+    Exception::ice_print(out);
+    out << ":\nillegal servant: `" << reason << "'";
+}
+
+
 static void
 printFailedRequestData(ostream& out, const RequestFailedException& ex)
 {
diff --git a/cpp/src/Ice/ObjectAdapterI.cpp b/cpp/src/Ice/ObjectAdapterI.cpp
index dbf3d5183e4..33b275bd616 100644
--- a/cpp/src/Ice/ObjectAdapterI.cpp
+++ b/cpp/src/Ice/ObjectAdapterI.cpp
@@ -44,6 +44,25 @@ using namespace std;
 using namespace Ice;
 using namespace IceInternal;
 
+namespace
+{
+inline void checkIdentity(const Identity& ident)
+{
+    if(ident.name.size() == 0)
+    {
+        throw IllegalIdentityException(__FILE__, __LINE__, ident);
+    }
+}
+
+inline void checkServant(const ObjectPtr& servant)
+{
+    if(servant == 0)
+    {
+        throw IllegalServantException(__FILE__, __LINE__, "cannot add null servant to Object Adapter");
+    }
+}
+}
+
 string
 Ice::ObjectAdapterI::getName() const
 {
@@ -428,6 +447,7 @@ Ice::ObjectAdapterI::addFacet(const ObjectPtr& object, const Identity& ident, co
     IceUtil::Monitor<IceUtil::RecMutex>::Lock sync(*this);
 
     checkForDeactivation();
+    checkServant(object);
     checkIdentity(ident);
 
     _servantManager->addServant(object, ident, facet);
@@ -452,10 +472,11 @@ Ice::ObjectAdapterI::addFacetWithUUID(const ObjectPtr& object, const string& fac
 void
 Ice::ObjectAdapterI::addDefaultServant(const ObjectPtr& servant, const string& category)
 {
+    checkServant(servant);
+
     IceUtil::Monitor<IceUtil::RecMutex>::Lock sync(*this);
 
     checkForDeactivation();
-
     _servantManager->addDefaultServant(servant, category);
 }
 
@@ -1140,17 +1161,6 @@ Ice::ObjectAdapterI::checkForDeactivation() const
     }
 }
 
-void
-Ice::ObjectAdapterI::checkIdentity(const Identity& ident)
-{
-    if(ident.name.size() == 0)
-    {
-        IllegalIdentityException e(__FILE__, __LINE__);
-        e.id = ident;
-        throw e;
-    }
-}
-
 vector<EndpointIPtr>
 Ice::ObjectAdapterI::parseEndpoints(const string& endpts, bool oaEndpoints) const
 {
diff --git a/cpp/src/Ice/ObjectAdapterI.h b/cpp/src/Ice/ObjectAdapterI.h
index 55946b580ef..52a3865c714 100644
--- a/cpp/src/Ice/ObjectAdapterI.h
+++ b/cpp/src/Ice/ObjectAdapterI.h
@@ -111,7 +111,6 @@ private:
     ObjectPrx newDirectProxy(const Identity&, const std::string&) const;
     ObjectPrx newIndirectProxy(const Identity&, const std::string&, const std::string&) const;
     void checkForDeactivation() const;
-    static void checkIdentity(const Identity&);
     std::vector<IceInternal::EndpointIPtr> parseEndpoints(const std::string&, bool) const;
     std::vector<IceInternal::EndpointIPtr> parsePublishedEndpoints();
     void updateLocatorRegistry(const IceInternal::LocatorInfoPtr&, const Ice::ObjectPrx&, bool);
diff --git a/cpp/test/Ice/exceptions/AllTests.cpp b/cpp/test/Ice/exceptions/AllTests.cpp
index 605d1f5f9a7..81a093f5c2c 100644
--- a/cpp/test/Ice/exceptions/AllTests.cpp
+++ b/cpp/test/Ice/exceptions/AllTests.cpp
@@ -1024,6 +1024,24 @@ allTests(const Ice::CommunicatorPtr& communicator)
         {
         }
 
+        try
+        {
+            adapter->add(obj, communicator->stringToIdentity(""));
+        }
+        catch(const Ice::IllegalIdentityException& ex)
+        {
+            test(ex.id.name == "");
+        }
+        
+        try
+        {
+            adapter->add(0, communicator->stringToIdentity("x"));
+        }
+        catch(const Ice::IllegalServantException&)
+        {
+        }
+
+
         adapter->remove(communicator->stringToIdentity("x"));
         try
         {
diff --git a/cs/src/Ice/ObjectAdapterI.cs b/cs/src/Ice/ObjectAdapterI.cs
index 3d3849b01c5..f6fc77bf68d 100644
--- a/cs/src/Ice/ObjectAdapterI.cs
+++ b/cs/src/Ice/ObjectAdapterI.cs
@@ -459,6 +459,7 @@ namespace Ice
             {
                 checkForDeactivation();
                 checkIdentity(ident);
+                checkServant(obj);
 
                 //
                 // Create a copy of the Identity argument, in case the caller
@@ -494,6 +495,8 @@ namespace Ice
 
         public void addDefaultServant(Ice.Object servant, string category)
         {
+            checkServant(servant);
+
             _m.Lock();
             try
             {
@@ -1346,10 +1349,7 @@ namespace Ice
         {
             if(ident.name == null || ident.name.Length == 0)
             {
-                IllegalIdentityException e = new IllegalIdentityException();
-                e.id.name = ident.name;
-                e.id.category = ident.category;
-                throw e;
+                throw new IllegalIdentityException(ident);
             }       
             if(ident.category == null)
             {
@@ -1357,6 +1357,14 @@ namespace Ice
             }
         }
 
+        private static void checkServant(Ice.Object servant)
+        {
+            if(servant == null)
+            {
+                throw new IllegalServantException("cannot add null servant to Object Adapter");
+            }
+        }
+
         private List<IceInternal.EndpointI> parseEndpoints(string endpts, bool oaEndpoints)
         {
             int beg;
diff --git a/cs/test/Ice/exceptions/AllTests.cs b/cs/test/Ice/exceptions/AllTests.cs
index acdf0ab6e2a..cebefc18d5b 100644
--- a/cs/test/Ice/exceptions/AllTests.cs
+++ b/cs/test/Ice/exceptions/AllTests.cs
@@ -1036,6 +1036,25 @@ public class AllTests : TestCommon.TestApp
             {
             }
 
+            try
+            {
+                adapter.add(obj, communicator.stringToIdentity(""));
+                test(false);
+            }
+            catch(Ice.IllegalIdentityException e)
+            {
+                test(e.id.name.Equals(""));
+            }
+
+            try
+            {
+                adapter.add(null, communicator.stringToIdentity("x"));
+                test(false);
+            }
+            catch(Ice.IllegalServantException)
+            {
+            }
+
             adapter.remove(communicator.stringToIdentity("x"));
             try
             {
diff --git a/java/src/Freeze/BackgroundSaveEvictorI.java b/java/src/Freeze/BackgroundSaveEvictorI.java
index da633b450f1..adb128e3c87 100644
--- a/java/src/Freeze/BackgroundSaveEvictorI.java
+++ b/java/src/Freeze/BackgroundSaveEvictorI.java
@@ -198,6 +198,7 @@ class BackgroundSaveEvictorI extends EvictorI implements BackgroundSaveEvictor,
     addFacet(Ice.Object servant, Ice.Identity ident, String facet)
     {
         checkIdentity(ident);
+        checkServant(servant);
 
         if(facet == null)
         {
diff --git a/java/src/Freeze/EvictorI.java b/java/src/Freeze/EvictorI.java
index 24575082d7e..86284eef996 100644
--- a/java/src/Freeze/EvictorI.java
+++ b/java/src/Freeze/EvictorI.java
@@ -561,9 +561,16 @@ abstract class EvictorI implements Evictor
     {
         if(ident.name == null || ident.name.length() == 0)
         {
-            Ice.IllegalIdentityException e = new Ice.IllegalIdentityException();
-            e.id = ident;
-            throw e;
+            throw new Ice.IllegalIdentityException(ident);
+        }
+    }
+
+    static void
+    checkServant(Ice.Object servant)
+    {
+        if(servant == null)
+        {
+            throw new Ice.IllegalServantException("cannot add null servant to Freeze Evictor");
         }
     }
 
diff --git a/java/src/Freeze/TransactionalEvictorI.java b/java/src/Freeze/TransactionalEvictorI.java
index 5d92342b61d..1e24e8a6319 100644
--- a/java/src/Freeze/TransactionalEvictorI.java
+++ b/java/src/Freeze/TransactionalEvictorI.java
@@ -52,6 +52,7 @@ class TransactionalEvictorI extends EvictorI implements TransactionalEvictor
     addFacet(Ice.Object servant, Ice.Identity ident, String facet)
     {
         checkIdentity(ident);
+        checkServant(servant);
 
         if(facet == null)
         {
diff --git a/java/src/Ice/ObjectAdapterI.java b/java/src/Ice/ObjectAdapterI.java
index c8760a558d7..05f79565a9d 100644
--- a/java/src/Ice/ObjectAdapterI.java
+++ b/java/src/Ice/ObjectAdapterI.java
@@ -423,6 +423,7 @@ public final class ObjectAdapterI implements ObjectAdapter
     {
         checkForDeactivation();
         checkIdentity(ident);
+        checkServant(object);
 
         //
         // Create a copy of the Identity argument, in case the caller
@@ -456,6 +457,7 @@ public final class ObjectAdapterI implements ObjectAdapter
     public synchronized void
     addDefaultServant(Ice.Object servant, String category)
     {
+        checkServant(servant);
         checkForDeactivation();
 
         _servantManager.addDefaultServant(servant, category);
@@ -1150,9 +1152,7 @@ public final class ObjectAdapterI implements ObjectAdapter
     {
         if(ident.name == null || ident.name.length() == 0)
         {
-            IllegalIdentityException e = new IllegalIdentityException();
-            e.id = (Identity)ident.clone();
-            throw e;
+            throw new IllegalIdentityException(ident);
         }
 
         if(ident.category == null)
@@ -1161,6 +1161,15 @@ public final class ObjectAdapterI implements ObjectAdapter
         }
     }
 
+    private static void
+    checkServant(Ice.Object servant)
+    {
+        if(servant == null)
+        {
+            throw new IllegalServantException("cannot add null servant to Object Adapter");
+        }
+    }
+
     private java.util.List<IceInternal.EndpointI>
     parseEndpoints(String endpts, boolean oaEndpoints)
     {
diff --git a/java/test/Ice/exceptions/AllTests.java b/java/test/Ice/exceptions/AllTests.java
index 0c0256b9bb7..bc556b0b5eb 100644
--- a/java/test/Ice/exceptions/AllTests.java
+++ b/java/test/Ice/exceptions/AllTests.java
@@ -1431,6 +1431,24 @@ public class AllTests
             {
             }
 
+            try
+            {
+                adapter.add(obj, communicator.stringToIdentity(""));
+                test(false);
+            }
+            catch(Ice.IllegalIdentityException ex)
+            {
+                test(ex.id.name.equals(""));
+            } 
+            try
+            {
+                adapter.add(null, communicator.stringToIdentity("x"));
+                test(false);
+            }
+            catch(Ice.IllegalServantException ex)
+            {
+            }
+            
             adapter.remove(communicator.stringToIdentity("x"));
             try
             {
diff --git a/js/src/Ice/ObjectAdapterI.js b/js/src/Ice/ObjectAdapterI.js
index 9bed2f5721a..c4f2022d68f 100644
--- a/js/src/Ice/ObjectAdapterI.js
+++ b/js/src/Ice/ObjectAdapterI.js
@@ -313,6 +313,7 @@
         {
             this.checkForDeactivation();
             this.checkIdentity(ident);
+            this.checkServant(object);
 
             //
             // Create a copy of the Identity argument, in case the caller
@@ -334,6 +335,7 @@
         },
         addDefaultServant: function(servant, category)
         {
+            this.checkServant(servant);
             this.checkForDeactivation();
 
             this._servantManager.addDefaultServant(servant, category);
@@ -493,9 +495,7 @@
         {
             if(ident.name === undefined || ident.name === null || ident.name.length === 0)
             {
-                var e = new Ice.IllegalIdentityException();
-                e.id = ident.clone();
-                throw e;
+                throw new Ice.IllegalIdentityException(ident);
             }
 
             if(ident.category === undefined || ident.category === null)
@@ -503,6 +503,13 @@
                 ident.category = "";
             }
         },
+        checkServant: function(servant)
+        {
+            if(servant === undefined || servant === null)
+            {
+                throw new Ice.IllegalServantException("cannot add null servant to Object Adapter");
+            }
+        },
         filterProperties: function(unknownProps)
         {
             //
diff --git a/js/test/Ice/exceptions/Client.js b/js/test/Ice/exceptions/Client.js
index 9d61f846dcd..6034cedb0a4 100644
--- a/js/test/Ice/exceptions/Client.js
+++ b/js/test/Ice/exceptions/Client.js
@@ -177,6 +177,25 @@
                         {
                             test(ex instanceof Ice.AlreadyRegisteredException);
                         }
+                        try
+                        {
+                            adapter.add(obj, communicator.stringToIdentity(""));
+                            test(false);
+                        }
+                        catch(ex)
+                        {
+                            test(ex instanceof Ice.IllegalIdentityException);
+                            test(ex.id.name == "");
+                        }
+                        try
+                        {
+                            adapter.add(null, communicator.stringToIdentity("x"));
+                            test(false);
+                        }
+                        catch(ex)
+                        {
+                            test(ex instanceof Ice.IllegalServantException);
+                        }
 
                         adapter.remove(communicator.stringToIdentity("x"));
                         try
diff --git a/slice/Ice/LocalException.ice b/slice/Ice/LocalException.ice
index 63118683814..7cb7403311c 100644
--- a/slice/Ice/LocalException.ice
+++ b/slice/Ice/LocalException.ice
@@ -403,6 +403,24 @@ local exception IllegalIdentityException
 
 /**
  *
+ * This exception is raised to reject an illegal servant (typically
+ * a null servant)
+ *
+ **/
+["cpp:ice_print"]
+local exception IllegalServantException
+{
+    /**
+     *
+     * Describes why this servant is illegal.
+     *
+     **/
+    string reason;
+};
+
+
+/**
+ *
  * This exception is raised if a request failed. This exception, and
  * all exceptions derived from {@link RequestFailedException}, are
  * transmitted by the Ice protocol, even though they are declared