Repository: python-zk/kazoo
Pull request #534 (2018-10-30 01:15:56)

fix: ensure timeout argument is nonnegative
Previously, a gap between calls to `time.time()` could lead to a situation where the current time was less than `end` during the `while` condition, but it was greater than `end` when assigning a value to `timeout_at`.

This is the same issue describe in https://github.com/python-zk/kazoo/pull/518, but the fix is slightly different. It avoids a redundant call to `time.time()`. I've also included a unit test that exhibits the problem when run without the fix.

cdonati: @StephenSorriaux Thanks for the review. Originally, I wasn't sure if (core) was applicable here for the scope, but that makes sense now.

In addition to the tests you suggested, I also added a couple tests for nonpositive timeout values.

Let me know if the amended commits address all of your comments.
cdonati: @StephenSorriaux Your changes look good to me. Would you prefer I use the "Apply suggestion" feature on all of them or should I push the changes you suggested as a single additional commit?
StephenSorriaux: @cdonati I think it is better that you push the changes, I'm not sure how many commits will be created using the "Apply suggestion" feature.
cdonati: @StephenSorriaux I just saw your comment about a single commit. I squashed the commits accordingly.
StephenSorriaux: @cdonati Thanks again for your PR, I'm merging it to master.

Patch:
diff --git a/kazoo/handlers/utils.py b/kazoo/handlers/utils.py
index 0d36506..2517390 100644
--- a/kazoo/handlers/utils.py
+++ b/kazoo/handlers/utils.py
@@ -198,8 +198,13 @@ def create_tcp_connection(module, address, timeout=None,
         end = time.time() + timeout
     sock = None
 
-    while end is None or time.time() < end:
+    while True:
         timeout_at = end if end is None else end - time.time()
+        # The condition is not '< 0' here because socket.settimeout treats 0 as
+        # a special case to put the socket in non-blocking mode.
+        if timeout_at is not None and timeout_at <= 0:
+            break
+
         if use_ssl:
             # Disallow use of SSLv2 and V3 (meaning we require TLSv1.0+)
             context = ssl.SSLContext(ssl.PROTOCOL_SSLv23)


Test patch:
diff --git a/kazoo/tests/test_utils.py b/kazoo/tests/test_utils.py
new file mode 100644
index 0000000..260d993
--- /dev/null
+++ b/kazoo/tests/test_utils.py
@@ -0,0 +1,78 @@
+import unittest
+
+from mock import patch
+from nose import SkipTest
+
+try:
+    from kazoo.handlers.eventlet import green_socket as socket
+    EVENTLET_HANDLER_AVAILABLE = True
+except ImportError:
+    EVENTLET_HANDLER_AVAILABLE = False
+
+
+class TestCreateTCPConnection(unittest.TestCase):
+    def test_timeout_arg(self):
+        from kazoo.handlers import utils
+        from kazoo.handlers.utils import create_tcp_connection, socket, time
+
+        with patch.object(socket, 'create_connection') as create_connection:
+            with patch.object(utils, '_set_default_tcpsock_options'):
+                # Ensure a gap between calls to time.time() does not result in
+                # create_connection being called with a negative timeout
+                # argument.
+                with patch.object(time, 'time', side_effect=range(10)):
+                    create_tcp_connection(socket, ('127.0.0.1', 2181),
+                                          timeout=1.5)
+
+                for call_args in create_connection.call_args_list:
+                    timeout = call_args[0][1]
+                    assert timeout >= 0, 'socket timeout must be nonnegative'
+
+    def test_timeout_arg_eventlet(self):
+        if not EVENTLET_HANDLER_AVAILABLE:
+            raise SkipTest('eventlet handler not available.')
+
+        from kazoo.handlers import utils
+        from kazoo.handlers.utils import create_tcp_connection, time
+
+        with patch.object(socket, 'create_connection') as create_connection:
+            with patch.object(utils, '_set_default_tcpsock_options'):
+                # Ensure a gap between calls to time.time() does not result in
+                # create_connection being called with a negative timeout
+                # argument.
+                with patch.object(time, 'time', side_effect=range(10)):
+                    create_tcp_connection(socket, ('127.0.0.1', 2181),
+                                          timeout=1.5)
+
+                for call_args in create_connection.call_args_list:
+                    timeout = call_args[0][1]
+                    assert timeout >= 0, 'socket timeout must be nonnegative'
+
+    def test_slow_connect(self):
+        # Currently, create_tcp_connection will raise a socket timeout if it
+        # takes longer than the specified "timeout" to create a connection.
+        # In the future, "timeout" might affect only the created socket and not
+        # the time it takes to create it.
+        from kazoo.handlers.utils import create_tcp_connection, socket, time
+
+        # Simulate a second passing between calls to check the current time.
+        with patch.object(time, 'time', side_effect=range(10)):
+            with self.assertRaises(socket.error):
+                create_tcp_connection(socket, ('127.0.0.1', 2181), timeout=0.5)
+
+    def test_negative_timeout(self):
+        from kazoo.handlers.utils import create_tcp_connection, socket
+        with self.assertRaises(socket.error):
+            create_tcp_connection(socket, ('127.0.0.1', 2181), timeout=-1)
+
+    def test_zero_timeout(self):
+        # Rather than pass '0' through as a timeout to
+        # socket.create_connection, create_tcp_connection should raise
+        # socket.error. This is because the socket library treats '0' as an
+        # indicator to create a non-blocking socket.
+        from kazoo.handlers.utils import create_tcp_connection, socket, time
+
+        # Simulate no time passing between calls to check the current time.
+        with patch.object(time, 'time', return_value=time.time()):
+            with self.assertRaises(socket.error):
+                create_tcp_connection(socket, ('127.0.0.1', 2181), timeout=0)