Add two more unit tests for SOCKS5:

  * Check that connecting again after having disconnected works.
  * Check that trying to connect with a very large hostname fails.

Also removes some unused code from socks_client_socket.cc.

BUG=NONE

Review URL: http://codereview.chromium.org/501112

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@35008 0039d316-1c4b-4281-b951-d872f2087c98

2 files changed, 47 insertions, 22 deletions

diff --git a/net/socket/socks5_client_socket.cc b/net/socket/socks5_client_socket.cc
index 7386caf..33d8e53 100644
--- a/net/socket/socks5_client_socket.cc
+++ b/net/socket/socks5_client_socket.cc
@@ -85,7 +85,7 @@ bool SOCKS5ClientSocket::IsConnectedAndIdle() const {
 // Read is called by the transport layer above to read. This can only be done
 // if the SOCKS handshake is complete.
 int SOCKS5ClientSocket::Read(IOBuffer* buf, int buf_len,
-                            CompletionCallback* callback) {
+                             CompletionCallback* callback) {
   DCHECK(completed_handshake_);
   DCHECK_EQ(STATE_NONE, next_state_);
   DCHECK(!user_callback_);
@@ -182,6 +182,11 @@ const char kSOCKS5GreetWriteData[] = { 0x05, 0x01, 0x00 };  // no authentication
 const char kSOCKS5GreetReadData[] = { 0x05, 0x00 };
 
 int SOCKS5ClientSocket::DoGreetWrite() {
+  // Since we only have 1 byte to send the hostname length in, if the
+  // URL has a hostname longer than 255 characters we can't send it.
+  if (0xFF < host_request_info_.hostname().size())
+    return ERR_INVALID_URL;
+
   if (buffer_.empty()) {
     buffer_ = std::string(kSOCKS5GreetWriteData,
                           arraysize(kSOCKS5GreetWriteData));
@@ -251,10 +256,7 @@ int SOCKS5ClientSocket::BuildHandshakeWriteBuffer(std::string* handshake)
 
   handshake->push_back(kEndPointDomain);  // The type of the address.
 
-  // We only have 1 byte to send the length in, so if the hostname is
-  // longer than this we can't send it!
-  if(256 <= host_request_info_.hostname().size())
-    return ERR_INVALID_URL;
+  DCHECK_GE(static_cast<size_t>(0xFF), host_request_info_.hostname().size());
 
   // First add the size of the hostname, followed by the hostname.
   handshake->push_back(static_cast<unsigned char>(
diff --git a/net/socket/socks5_client_socket_unittest.cc b/net/socket/socks5_client_socket_unittest.cc
index ae1b2c3..ac7ff5a 100644
--- a/net/socket/socks5_client_socket_unittest.cc
+++ b/net/socket/socks5_client_socket_unittest.cc
@@ -4,6 +4,7 @@
 
 #include "net/socket/socks5_client_socket.h"
 
+#include <algorithm>
 #include <map>
 
 #include "net/base/address_list.h"
@@ -153,9 +154,8 @@ TEST_F(SOCKS5ClientSocketTest, CompleteHandshake) {
   EXPECT_FALSE(user_sock_->IsConnected());
 }
 
-// Connect to a domain, making sure to defer the host resolving to the proxy
-// server.
-TEST_F(SOCKS5ClientSocketTest, ResolveHostsProxySide) {
+// Test that you can call Connect() again after having called Disconnect().
+TEST_F(SOCKS5ClientSocketTest, ConnectAndDisconnectTwice) {
   const std::string hostname = "my-host-name";
   const char kSOCKS5DomainRequest[] = {
       0x05,  // VER
@@ -164,26 +164,49 @@ TEST_F(SOCKS5ClientSocketTest, ResolveHostsProxySide) {
       0x03,  // ATYPE
   };
 
-  std::string request(kSOCKS5DomainRequest,
-                      arraysize(kSOCKS5DomainRequest));
+  std::string request(kSOCKS5DomainRequest, arraysize(kSOCKS5DomainRequest));
   request.push_back(hostname.size());
   request.append(hostname);
   request.append(reinterpret_cast<const char*>(&kNwPort), sizeof(kNwPort));
 
-  MockWrite data_writes[] = {
-      MockWrite(false, kSOCKS5GreetRequest, arraysize(kSOCKS5GreetRequest)),
-      MockWrite(false, request.data(), request.size())
-  };
-  MockRead data_reads[] = {
-      MockRead(false, kSOCKS5GreetResponse, arraysize(kSOCKS5GreetResponse)),
-      MockRead(false, kSOCKS5OkResponse, arraysize(kSOCKS5OkResponse))
-  };
+  for (int i = 0; i < 2; ++i) {
+    MockWrite data_writes[] = {
+        MockWrite(false, kSOCKS5GreetRequest, arraysize(kSOCKS5GreetRequest)),
+        MockWrite(false, request.data(), request.size())
+    };
+    MockRead data_reads[] = {
+        MockRead(false, kSOCKS5GreetResponse, arraysize(kSOCKS5GreetResponse)),
+        MockRead(false, kSOCKS5OkResponse, arraysize(kSOCKS5OkResponse))
+    };
 
-  user_sock_.reset(BuildMockSocket(data_reads, data_writes, hostname, 80));
+    user_sock_.reset(BuildMockSocket(data_reads, data_writes, hostname, 80));
 
-  int rv = user_sock_->Connect(&callback_, NULL);
-  EXPECT_EQ(OK, rv);
-  EXPECT_TRUE(user_sock_->IsConnected());
+    int rv = user_sock_->Connect(&callback_, NULL);
+    EXPECT_EQ(OK, rv);
+    EXPECT_TRUE(user_sock_->IsConnected());
+
+    user_sock_->Disconnect();
+    EXPECT_FALSE(user_sock_->IsConnected());
+  }
+}
+
+// Test that we fail trying to connect to a hosname longer than 255 bytes.
+TEST_F(SOCKS5ClientSocketTest, LargeHostNameFails) {
+  // Create a string of length 256, where each character is 'x'.
+  std::string large_host_name;
+  std::fill_n(std::back_inserter(large_host_name), 256, 'x');
+
+  // Create a SOCKS socket, with mock transport socket.
+  MockWrite data_writes[] = {MockWrite()};
+  MockRead data_reads[] = {MockRead()};
+  user_sock_.reset(BuildMockSocket(data_reads, data_writes,
+                                   large_host_name, 80));
+
+  // Try to connect -- should fail (without having read/written anything to
+  // the transport socket first) because the hostname is too long.
+  TestCompletionCallback callback;
+  int rv = user_sock_->Connect(&callback, NULL);
+  EXPECT_EQ(ERR_INVALID_URL, rv);
 }
 
 TEST_F(SOCKS5ClientSocketTest, PartialReadWrites) {