Use Socket.tcp instead of TCPSocket.new to provide a connect_timeout

This patch prevents LDAP connections to hang up for an eccessive amount of time
and instead returns earlier in case of failures (e.g., packets dropped).

Author: Stefano Tortarolo

lib/net/ldap.rb

@@ -1243,7 +1243,7 @@ def new_connection
       :hosts                   => @hosts,
       :encryption              => @encryption,
       :instrumentation_service => @instrumentation_service
-  rescue Errno::ECONNREFUSED, Net::LDAP::ConnectionRefusedError => e
+  rescue Errno::ECONNREFUSED, Errno::ETIMEDOUT, Net::LDAP::ConnectionRefusedError => e
     @result = {
       :resultCode   => 52,
       :errorMessage => ResultStrings[ResultCodeUnavailable]

lib/net/ldap/connection.rb

@@ -3,6 +3,9 @@
 class Net::LDAP::Connection #:nodoc:
   include Net::LDAP::Instrumentation
 
+  # Number of seconds to wait for a socket#connect success
+  ConnectTimeout = 1
+
   LdapVersion = 3
   MaxSaslChallenges = 10
 
@@ -34,7 +37,7 @@ def open_connection(server)
     errors = []
     hosts.each do |host, port|
       begin
-        prepare_socket(server.merge(socket: TCPSocket.new(host, port)))
+        prepare_socket(server.merge(socket: Socket.tcp(host, port, connect_timeout: ConnectTimeout)))
         return
       rescue Net::LDAP::Error, SocketError, SystemCallError,
              OpenSSL::SSL::SSLError => e

test/test_auth_adapter.rb

@@ -2,7 +2,7 @@
 
 class TestAuthAdapter < Test::Unit::TestCase
   def test_undefined_auth_adapter
-    flexmock(TCPSocket).should_receive(:new).ordered.with('ldap.example.com', 379).once.and_return(nil)
+    flexmock(Socket).should_receive(:tcp).ordered.with('ldap.example.com', 379, { connect_timeout: 1 }).once.and_return(nil)
     conn = Net::LDAP::Connection.new(host: 'ldap.example.com', port: 379)
     assert_raise Net::LDAP::AuthMethodUnsupportedError, "Unsupported auth method (foo)" do
       conn.bind(method: :foo)

test/test_ldap_connection.rb

@@ -15,8 +15,8 @@ def test_list_of_hosts_with_first_host_successful
       ['test2.mocked.com', 636],
       ['test3.mocked.com', 636],
     ]
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_return(nil)
-    flexmock(TCPSocket).should_receive(:new).ordered.never
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 1 }).once.and_return(nil)
+    flexmock(Socket).should_receive(:tcp).ordered.never
     Net::LDAP::Connection.new(:hosts => hosts)
   end
 
@@ -26,9 +26,9 @@ def test_list_of_hosts_with_first_host_failure
       ['test2.mocked.com', 636],
       ['test3.mocked.com', 636],
     ]
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_return(nil)
-    flexmock(TCPSocket).should_receive(:new).ordered.never
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 1 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 1 }).once.and_return(nil)
+    flexmock(Socket).should_receive(:tcp).ordered.never
     Net::LDAP::Connection.new(:hosts => hosts)
   end
 
@@ -38,17 +38,17 @@ def test_list_of_hosts_with_all_hosts_failure
       ['test2.mocked.com', 636],
       ['test3.mocked.com', 636],
     ]
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[0]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[1]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.with(*hosts[2]).once.and_raise(SocketError)
-    flexmock(TCPSocket).should_receive(:new).ordered.never
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[0], { connect_timeout: 1 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[1], { connect_timeout: 1 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.with(*hosts[2], { connect_timeout: 1 }).once.and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).ordered.never
     assert_raise Net::LDAP::ConnectionError do
       Net::LDAP::Connection.new(:hosts => hosts)
     end
   end
 
   def test_result_for_connection_failed_is_set
-    flexmock(TCPSocket).should_receive(:new).and_raise(Errno::ECONNREFUSED)
+    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)
 
     ldap_client = Net::LDAP.new(host: '127.0.0.1', port: 12345)
 
@@ -67,14 +67,14 @@ def test_unresponsive_host
   end
 
   def test_blocked_port
-    flexmock(TCPSocket).should_receive(:new).and_raise(SocketError)
+    flexmock(Socket).should_receive(:tcp).and_raise(SocketError)
     assert_raise Net::LDAP::Error do
       Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
     end
   end
 
   def test_connection_refused
-    flexmock(TCPSocket).should_receive(:new).and_raise(Errno::ECONNREFUSED)
+    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ECONNREFUSED)
     stderr = capture_stderr do
       assert_raise Net::LDAP::ConnectionRefusedError do
         Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
@@ -83,9 +83,18 @@ def test_connection_refused
     assert_equal("Deprecation warning: Net::LDAP::ConnectionRefused will be deprecated. Use Errno::ECONNREFUSED instead.\n",  stderr)
   end
 
+  def test_connection_timedout
+    flexmock(Socket).should_receive(:tcp).and_raise(Errno::ETIMEDOUT)
+    stderr = capture_stderr do
+      assert_raise Net::LDAP::Error do
+        Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
+      end
+    end
+  end
+
   def test_raises_unknown_exceptions
     error = Class.new(StandardError)
-    flexmock(TCPSocket).should_receive(:new).and_raise(error)
+    flexmock(Socket).should_receive(:tcp).and_raise(error)
     assert_raise error do
       Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
     end
@@ -328,7 +337,7 @@ class TestLDAPConnectionErrors < Test::Unit::TestCase
   def setup
     @tcp_socket = flexmock(:connection)
     @tcp_socket.should_receive(:write)
-    flexmock(TCPSocket).should_receive(:new).and_return(@tcp_socket)
+    flexmock(Socket).should_receive(:tcp).and_return(@tcp_socket)
     @connection = Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
   end
 
@@ -357,7 +366,7 @@ class TestLDAPConnectionInstrumentation < Test::Unit::TestCase
   def setup
     @tcp_socket = flexmock(:connection)
     @tcp_socket.should_receive(:write)
-    flexmock(TCPSocket).should_receive(:new).and_return(@tcp_socket)
+    flexmock(Socket).should_receive(:tcp).and_return(@tcp_socket)
 
     @service = MockInstrumentationService.new
     @connection = Net::LDAP::Connection.new \