Added sanity checks for ACM timeout value (ICE-8749)

22 files changed, 205 insertions, 17 deletions

diff --git a/cpp/src/Ice/ACM.cpp b/cpp/src/Ice/ACM.cpp
index b8e9878309a..cc20ccc070d 100644
--- a/cpp/src/Ice/ACM.cpp
+++ b/cpp/src/Ice/ACM.cpp
@@ -45,8 +45,17 @@ IceInternal::ACMConfig::ACMConfig(const Ice::PropertiesPtr& p,
         timeoutProperty = prefix + ".Timeout";
     };
 
-    this->timeout = IceUtil::Time::seconds(p->getPropertyAsIntWithDefault(timeoutProperty,
-                                                                          static_cast<int>(dflt.timeout.toSeconds())));
+    int timeout = p->getPropertyAsIntWithDefault(timeoutProperty, static_cast<int>(dflt.timeout.toSeconds()));
+    if(timeout >= 0)
+    {
+        this->timeout = IceUtil::Time::seconds(timeout);
+    }
+    else
+    {
+        l->warning("invalid value for property `" + timeoutProperty + "', default value will be used instead");
+        this->timeout = dflt.timeout;
+    }
+
     int hb = p->getPropertyAsIntWithDefault(prefix + ".Heartbeat", static_cast<int>(dflt.heartbeat));
     if(hb >= static_cast<int>(ICE_ENUM(ACMHeartbeat, HeartbeatOff)) &&
        hb <= static_cast<int>(ICE_ENUM(ACMHeartbeat, HeartbeatAlways)))
diff --git a/cpp/src/Ice/ConnectionI.cpp b/cpp/src/Ice/ConnectionI.cpp
index d584d6ccd44..1f3bb85464c 100644
--- a/cpp/src/Ice/ConnectionI.cpp
+++ b/cpp/src/Ice/ConnectionI.cpp
@@ -1147,6 +1147,14 @@ Ice::ConnectionI::setACM(const IceUtil::Optional<int>& timeout,
                          const IceUtil::Optional<Ice::ACMHeartbeat>& heartbeat)
 {
     IceUtil::Monitor<IceUtil::Mutex>::Lock sync(*this);
+    if(timeout && *timeout < 0)
+    {
+#ifdef ICE_CPP11_MAPPING
+        throw invalid_argument("invalid negative ACM timeout value");
+#else
+        throw IceUtil::IllegalArgumentException(__FILE__, __LINE__, "invalid negative ACM timeout value");
+#endif
+    }
     if(!_monitor || _state >= StateClosed)
     {
         return;
diff --git a/cpp/test/Ice/acm/AllTests.cpp b/cpp/test/Ice/acm/AllTests.cpp
index d362b3b6bc6..94b8c0da84d 100644
--- a/cpp/test/Ice/acm/AllTests.cpp
+++ b/cpp/test/Ice/acm/AllTests.cpp
@@ -314,7 +314,7 @@ public:
         proxy->sleep(4);
 
         Lock sync(*this);
-        test(_heartbeat >= 6);
+        test(_heartbeat >= 4);
     }
 };
 
@@ -618,6 +618,21 @@ public:
     {
         Ice::ConnectionPtr con = proxy->ice_getConnection();
 
+        try
+        {
+            con->setACM(-19, IceUtil::None, IceUtil::None);
+            test(false);
+        }
+#ifndef ICE_CPP11_MAPPING
+        catch(const IceUtil::IllegalArgumentException&)
+        {
+        }
+#else
+        catch(const invalid_argument&)
+        {
+        }
+#endif
+
         Ice::ACM acm;
         acm = con->getACM();
         test(acm.timeout == 15);
diff --git a/csharp/src/Ice/ACM.cs b/csharp/src/Ice/ACM.cs
index 20eca46540e..8a0811b5fa7 100644
--- a/csharp/src/Ice/ACM.cs
+++ b/csharp/src/Ice/ACM.cs
@@ -36,7 +36,12 @@ namespace IceInternal
                 timeoutProperty = prefix + ".Timeout";
             }
 
-            timeout = p.getPropertyAsIntWithDefault(timeoutProperty, dflt.timeout / 1000) * 1000;
+            timeout = p.getPropertyAsIntWithDefault(timeoutProperty, dflt.timeout / 1000) * 1000; // To milliseconds
+            if(timeout < 0)
+            {
+                l.warning("invalid value for property `" + timeoutProperty + "', default value will be used instead");
+                timeout = dflt.timeout;
+            }
 
             int hb = p.getPropertyAsIntWithDefault(prefix + ".Heartbeat", (int)dflt.heartbeat);
             if(hb >= (int)Ice.ACMHeartbeat.HeartbeatOff && hb <= (int)Ice.ACMHeartbeat.HeartbeatAlways)
diff --git a/csharp/src/Ice/ConnectionI.cs b/csharp/src/Ice/ConnectionI.cs
index ec99e3d400b..ad6322dbcf8 100644
--- a/csharp/src/Ice/ConnectionI.cs
+++ b/csharp/src/Ice/ConnectionI.cs
@@ -748,6 +748,10 @@ namespace Ice
         {
             lock(this)
             {
+                if(timeout.HasValue && timeout.Value < 0)
+                {
+                    throw new ArgumentException("invalid negative ACM timeout value");
+                }
                 if(_monitor == null || _state >= StateClosed)
                 {
                     return;
diff --git a/csharp/test/Ice/acm/AllTests.cs b/csharp/test/Ice/acm/AllTests.cs
index 81c8ce5a33c..a4442296124 100644
--- a/csharp/test/Ice/acm/AllTests.cs
+++ b/csharp/test/Ice/acm/AllTests.cs
@@ -8,6 +8,7 @@
 // **********************************************************************
 
 using System;
+using System.Globalization;
 using System.Collections.Generic;
 using System.Threading;
 using System.Threading.Tasks;
@@ -15,6 +16,11 @@ using Test;
 
 class LoggerI : Ice.Logger
 {
+    public LoggerI(string name)
+    {
+        _name = name;
+    }
+
     public void start()
     {
         lock(this)
@@ -40,7 +46,17 @@ class LoggerI : Ice.Logger
     {
         lock(this)
         {
-            _messages.Add("[" + category + "] " + message);
+            System.Text.StringBuilder s = new System.Text.StringBuilder(_name);
+            s.Append(' ');
+            s.Append(System.DateTime.Now.ToString(_date, CultureInfo.CurrentCulture));
+            s.Append(' ');
+            s.Append(System.DateTime.Now.ToString(_time, CultureInfo.CurrentCulture));
+            s.Append(' ');
+            s.Append("[");
+            s.Append(category);
+            s.Append("] ");
+            s.Append(message);
+            _messages.Add(s.ToString());
             if(_started)
             {
                 dump();
@@ -52,7 +68,14 @@ class LoggerI : Ice.Logger
     {
         lock(this)
         {
-            _messages.Add("warning: " + message);
+            System.Text.StringBuilder s = new System.Text.StringBuilder(_name);
+            s.Append(' ');
+            s.Append(System.DateTime.Now.ToString(_date, CultureInfo.CurrentCulture));
+            s.Append(' ');
+            s.Append(System.DateTime.Now.ToString(_time, CultureInfo.CurrentCulture));
+            s.Append(" warning : ");
+            s.Append(message);
+            _messages.Add(s.ToString());
             if(_started)
             {
                 dump();
@@ -64,7 +87,14 @@ class LoggerI : Ice.Logger
     {
         lock(this)
         {
-            _messages.Add("error: " + message);
+            System.Text.StringBuilder s = new System.Text.StringBuilder(_name);
+            s.Append(' ');
+            s.Append(System.DateTime.Now.ToString(_date, CultureInfo.CurrentCulture));
+            s.Append(' ');
+            s.Append(System.DateTime.Now.ToString(_time, CultureInfo.CurrentCulture));
+            s.Append(" error : ");
+            s.Append(message);
+            _messages.Add(s.ToString());
             if(_started)
             {
                 dump();
@@ -86,12 +116,16 @@ class LoggerI : Ice.Logger
     {
         foreach(string line in _messages)
         {
-            System.Console.WriteLine(line);
+            System.Console.Error.WriteLine(line);
         }
         _messages.Clear();
     }
 
+    private string _name;
     private bool _started;
+    private readonly static string _date = "d";
+    private readonly static string _time = "HH:mm:ss:fff";
+
     private List<string> _messages = new List<string>();
 }
 
@@ -101,7 +135,7 @@ abstract class TestCase
     {
         _name = name;
         _com = com;
-        _logger = new LoggerI();
+        _logger = new LoggerI(_name);
 
         _clientACMTimeout = -1;
         _clientACMClose = -1;
@@ -268,7 +302,7 @@ public class AllTests : TestCommon.AllTests
 
             lock(this)
             {
-                test(_heartbeat >= 6);
+                test(_heartbeat >= 4);
             }
         }
     }
@@ -518,6 +552,15 @@ public class AllTests : TestCommon.AllTests
         {
             Ice.Connection con = proxy.ice_getCachedConnection();
 
+            try
+            {
+                con.setACM(-19, Ice.Util.None, Ice.Util.None);
+                test(false);
+            }
+            catch(ArgumentException)
+            {
+            }
+
             Ice.ACM acm;
             acm = con.getACM();
             test(acm.timeout == 15);
diff --git a/java-compat/src/Ice/src/main/java/Ice/ConnectionI.java b/java-compat/src/Ice/src/main/java/Ice/ConnectionI.java
index 9c4c4c45e16..8b334e9db6e 100644
--- a/java-compat/src/Ice/src/main/java/Ice/ConnectionI.java
+++ b/java-compat/src/Ice/src/main/java/Ice/ConnectionI.java
@@ -701,6 +701,10 @@ public final class ConnectionI extends IceInternal.EventHandler
     synchronized public void setACM(Ice.IntOptional timeout, Ice.Optional<ACMClose> close,
             Ice.Optional<ACMHeartbeat> heartbeat)
     {
+        if(timeout != null && timeout.isSet() && timeout.get() < 0)
+        {
+            throw new IllegalArgumentException("invalid negative ACM timeout value");
+        }
         if(_monitor == null || _state >= StateClosed)
         {
             return;
diff --git a/java-compat/src/Ice/src/main/java/IceInternal/ACMConfig.java b/java-compat/src/Ice/src/main/java/IceInternal/ACMConfig.java
index 7864e99f88e..3c34b4cbfc0 100644
--- a/java-compat/src/Ice/src/main/java/IceInternal/ACMConfig.java
+++ b/java-compat/src/Ice/src/main/java/IceInternal/ACMConfig.java
@@ -34,6 +34,11 @@ public final class ACMConfig implements java.lang.Cloneable
         }
 
         timeout = p.getPropertyAsIntWithDefault(timeoutProperty, dflt.timeout / 1000) * 1000; // To milliseconds
+        if(timeout < 0)
+        {
+            l.warning("invalid value for property `" + timeoutProperty + "', default value will be used instead");
+            timeout = dflt.timeout;
+        }
 
         int hb = p.getPropertyAsIntWithDefault(prefix + ".Heartbeat", dflt.heartbeat.ordinal());
         Ice.ACMHeartbeat[] heartbeatValues = Ice.ACMHeartbeat.values();
diff --git a/java-compat/test/src/main/java/test/Ice/acm/AllTests.java b/java-compat/test/src/main/java/test/Ice/acm/AllTests.java
index ca986b5eccb..9e420a3148c 100644
--- a/java-compat/test/src/main/java/test/Ice/acm/AllTests.java
+++ b/java-compat/test/src/main/java/test/Ice/acm/AllTests.java
@@ -316,7 +316,7 @@ public class AllTests
         public void runTestCase(RemoteObjectAdapterPrx adapter, TestIntfPrx proxy)
         {
             proxy.sleep(4);
-            test(_heartbeat >= 6);
+            test(_heartbeat >= 4);
         }
     }
 
@@ -614,6 +614,15 @@ public class AllTests
         {
             Ice.Connection con = proxy.ice_getConnection();
 
+            try
+            {
+                con.setACM(new Ice.IntOptional(-19), null, null);
+                test(false);
+            }
+            catch(IllegalArgumentException ex)
+            {
+            }
+
             Ice.ACM acm = new Ice.ACM();
             acm = con.getACM();
             test(acm.timeout == 15);
diff --git a/java/src/Ice/src/main/java/com/zeroc/Ice/ConnectionI.java b/java/src/Ice/src/main/java/com/zeroc/Ice/ConnectionI.java
index e984cc4703c..5671ea5409a 100644
--- a/java/src/Ice/src/main/java/com/zeroc/Ice/ConnectionI.java
+++ b/java/src/Ice/src/main/java/com/zeroc/Ice/ConnectionI.java
@@ -607,6 +607,10 @@ public final class ConnectionI extends com.zeroc.IceInternal.EventHandler
     synchronized public void setACM(java.util.OptionalInt timeout, java.util.Optional<ACMClose> close,
             java.util.Optional<ACMHeartbeat> heartbeat)
     {
+        if(timeout != null && timeout.isPresent() && timeout.getAsInt() < 0)
+        {
+            throw new IllegalArgumentException("invalid negative ACM timeout value");
+        }
         if(_monitor == null || _state >= StateClosed)
         {
             return;
diff --git a/java/src/Ice/src/main/java/com/zeroc/IceInternal/ACMConfig.java b/java/src/Ice/src/main/java/com/zeroc/IceInternal/ACMConfig.java
index c4f2009be8f..d8f1b1a64e5 100644
--- a/java/src/Ice/src/main/java/com/zeroc/IceInternal/ACMConfig.java
+++ b/java/src/Ice/src/main/java/com/zeroc/IceInternal/ACMConfig.java
@@ -37,6 +37,11 @@ public final class ACMConfig implements java.lang.Cloneable
         }
 
         timeout = p.getPropertyAsIntWithDefault(timeoutProperty, dflt.timeout / 1000) * 1000; // To milliseconds
+        if(timeout < 0)
+        {
+            l.warning("invalid value for property `" + timeoutProperty + "', default value will be used instead");
+            timeout = dflt.timeout;
+        }
 
         int hb = p.getPropertyAsIntWithDefault(prefix + ".Heartbeat", dflt.heartbeat.ordinal());
         ACMHeartbeat[] heartbeatValues = ACMHeartbeat.values();
diff --git a/java/test/src/main/java/test/Ice/acm/AllTests.java b/java/test/src/main/java/test/Ice/acm/AllTests.java
index c99c3b95b59..0403ac094fe 100644
--- a/java/test/src/main/java/test/Ice/acm/AllTests.java
+++ b/java/test/src/main/java/test/Ice/acm/AllTests.java
@@ -302,7 +302,7 @@ public class AllTests
         public void runTestCase(RemoteObjectAdapterPrx adapter, TestIntfPrx proxy)
         {
             proxy.sleep(4);
-            test(_heartbeat >= 6);
+            test(_heartbeat >= 4);
         }
     }
 
@@ -600,6 +600,15 @@ public class AllTests
         {
             com.zeroc.Ice.Connection con = proxy.ice_getConnection();
 
+            try
+            {
+                con.setACM(java.util.OptionalInt.of(-19), null, null);
+                test(false);
+            }
+            catch(IllegalArgumentException ex)
+            {
+            }
+
             com.zeroc.Ice.ACM acm = new com.zeroc.Ice.ACM();
             acm = con.getACM();
             test(acm.timeout == 15);
diff --git a/js/src/Ice/ACM.js b/js/src/Ice/ACM.js
index 62752c36d0b..871cb999fa1 100644
--- a/js/src/Ice/ACM.js
+++ b/js/src/Ice/ACM.js
@@ -36,6 +36,11 @@ class ACMConfig
         }
 
         this.timeout = p.getPropertyAsIntWithDefault(timeoutProperty, dflt.timeout / 1000) * 1000; // To ms
+        if(this.timeout < 0)
+        {
+            l.warning("invalid value for property `" + timeoutProperty + "', default value will be used instead");
+            this.timeout = dflt.timeout;
+        }
 
         const hb = p.getPropertyAsIntWithDefault(prefix + ".Heartbeat", dflt.heartbeat.value);
         if(hb >= 0 && hb <= Ice.ACMHeartbeat.maxValue)
diff --git a/js/src/Ice/ConnectionI.js b/js/src/Ice/ConnectionI.js
index 33996e61ad4..351375c7635 100644
--- a/js/src/Ice/ConnectionI.js
+++ b/js/src/Ice/ConnectionI.js
@@ -502,6 +502,10 @@ class ConnectionI
 
     setACM(timeout, close, heartbeat)
     {
+        if(timeout !== undefined && timeout < 0)
+        {
+            throw new Error("invalid negative ACM timeout value");
+        }
         if(this._monitor === null || this._state >= StateClosed)
         {
             return;
diff --git a/js/test/Ice/acm/Client.js b/js/test/Ice/acm/Client.js
index c86676a9553..8da91a6c2d9 100644
--- a/js/test/Ice/acm/Client.js
+++ b/js/test/Ice/acm/Client.js
@@ -191,7 +191,7 @@
         async runTestCase(adapter, proxy)
         {
             await proxy.sleep(4);
-            test(this._heartbeat >= 6);
+            test(this._heartbeat >= 4);
         }
     }
 
@@ -415,6 +415,15 @@
         {
             let con = proxy.ice_getCachedConnection();
 
+            try
+            {
+                con.setACM(-19, undefined, undefined);
+                test(false);
+            }
+            catch(ex)
+            {
+            }
+
             let acm = con.getACM();
             test(acm.timeout === 15);
             test(acm.close === Ice.ACMClose.CloseOnIdleForceful);
diff --git a/matlab/test/Ice/acm/AllTests.m b/matlab/test/Ice/acm/AllTests.m
index cf3c1238b46..a32fbf1e76c 100644
--- a/matlab/test/Ice/acm/AllTests.m
+++ b/matlab/test/Ice/acm/AllTests.m
@@ -41,6 +41,12 @@ classdef AllTests
             proxy = TestIntfPrx.uncheckedCast(testCommunicator.stringToProxy(adapter.getTestIntf().ice_toString()));
             proxy.ice_getConnection();
 
+            try
+                proxy.ice_getCachedConnection().setACM(-19, Ice.Unset, Ice.Unset);
+            catch ex
+                assert(false);
+            end
+
             acm = proxy.ice_getCachedConnection().getACM();
             assert(acm.timeout == 15);
             assert(acm.close == Ice.ACMClose.CloseOnIdleForceful);
diff --git a/objective-c/src/Ice/ConnectionI.mm b/objective-c/src/Ice/ConnectionI.mm
index 224520f0623..aea1b336f3d 100644
--- a/objective-c/src/Ice/ConnectionI.mm
+++ b/objective-c/src/Ice/ConnectionI.mm
@@ -446,7 +446,17 @@ private:
         hb = (Ice::ACMHeartbeat)intValue;
     }
 
-    CONNECTION->setACM(to, c, hb);
+    NSException* nsex;
+    try
+    {
+        CONNECTION->setACM(to, c, hb);
+        return;
+    }
+    catch(const std::exception& ex)
+    {
+        nsex = toObjCException(ex);
+    }
+    @throw nsex;
 }
 -(ICEACM*) getACM
 {
diff --git a/objective-c/test/Ice/acm/AllTests.m b/objective-c/test/Ice/acm/AllTests.m
index 1013718b85f..d713f90340c 100644
--- a/objective-c/test/Ice/acm/AllTests.m
+++ b/objective-c/test/Ice/acm/AllTests.m
@@ -400,7 +400,7 @@
     [_cond lock];
     @try
     {
-        test(_heartbeat >= 6);
+        test(_heartbeat >= 4);
     }
     @finally
     {
@@ -811,6 +811,16 @@
 }
 -(void) runTestCase:(id<TestACMRemoteObjectAdapterPrx>)adapter proxy:(id<TestACMTestIntfPrx>)proxy
 {
+    @try
+    {
+        [[proxy ice_getCachedConnection] setACM:@(-19) close:ICENone heartbeat:ICENone];
+        test(NO);
+    }
+    @catch(NSException* ex)
+    {
+        test([ex name] == NSInvalidArgumentException);
+    }
+
     ICEACM* acm = [[proxy ice_getCachedConnection] getACM];
     test(acm.timeout == 15);
     test(acm.close == ICECloseOnIdleForceful);
diff --git a/python/modules/IcePy/Connection.cpp b/python/modules/IcePy/Connection.cpp
index be94a61f508..f083e6ced6b 100644
--- a/python/modules/IcePy/Connection.cpp
+++ b/python/modules/IcePy/Connection.cpp
@@ -945,6 +945,11 @@ connectionSetACM(ConnectionObject* self, PyObject* args)
     {
         (*self->connection)->setACM(timeout, close, heartbeat);
     }
+    catch(const IceUtil::IllegalArgumentException& ex)
+    {
+        PyErr_Format(PyExc_RuntimeError, "%s", STRCAST(ex.reason().c_str()));
+        return 0;
+    }
     catch(const Ice::Exception& ex)
     {
         setPythonException(ex);
diff --git a/python/test/Ice/acm/AllTests.py b/python/test/Ice/acm/AllTests.py
index 8a694efbe67..29d3209b650 100644
--- a/python/test/Ice/acm/AllTests.py
+++ b/python/test/Ice/acm/AllTests.py
@@ -165,7 +165,7 @@ def allTests(communicator):
             proxy.sleep(4)
 
             with self.m:
-                test(self._heartbeat >= 6)
+                test(self._heartbeat >= 4)
 
     class InvocationHeartbeatOnHoldTest(TestCase):
         def __init__(self, com):
@@ -328,6 +328,12 @@ def allTests(communicator):
             self.setClientACM(15, 4, 0)
 
         def runTestCase(self, adapter, proxy):
+            try:
+                proxy.ice_getCachedConnection().setACM(-19, Ice.Unset, Ice.Unset)
+                test(False)
+            except RuntimeError:
+                pass
+
             acm = proxy.ice_getCachedConnection().getACM()
             test(acm.timeout == 15)
             test(acm.close == Ice.ACMClose.CloseOnIdleForceful)
diff --git a/ruby/src/IceRuby/Connection.cpp b/ruby/src/IceRuby/Connection.cpp
index 1f5c627c1dc..3d1398e8778 100644
--- a/ruby/src/IceRuby/Connection.cpp
+++ b/ruby/src/IceRuby/Connection.cpp
@@ -151,7 +151,14 @@ IceRuby_Connection_setACM(VALUE self, VALUE t, VALUE c, VALUE h)
             heartbeat = static_cast<Ice::ACMHeartbeat>(FIX2LONG(heartbeatValue));
         }
 
-        (*p)->setACM(timeout, close, heartbeat);
+        try
+        {
+            (*p)->setACM(timeout, close, heartbeat);
+        }
+        catch(const IceUtil::IllegalArgumentException& ex)
+        {
+            throw RubyException(rb_eArgError, ex.reason().c_str());
+        }
     }
     ICE_RUBY_CATCH
     return Qnil;
diff --git a/ruby/test/Ice/acm/AllTests.rb b/ruby/test/Ice/acm/AllTests.rb
index 416fadad76f..8b307d528c5 100644
--- a/ruby/test/Ice/acm/AllTests.rb
+++ b/ruby/test/Ice/acm/AllTests.rb
@@ -23,6 +23,12 @@ def testSetACM(communicator, com)
     proxy = Test::TestIntfPrx::uncheckedCast(testCommunicator.stringToProxy(adapter.getTestIntf().ice_toString()))
     proxy.ice_getConnection()
 
+    begin
+        proxy.ice_getCachedConnection().setACM(-19, Ice::Unset, Ice::Unset)
+        test(false)
+    rescue
+    end
+
     acm = proxy.ice_getCachedConnection().getACM()
     test(acm.timeout == 15)
     test(acm.close == Ice::ACMClose::CloseOnIdleForceful)