[Sync] Make ChromeAsyncSocket use only the hostname on connect

jingle/notifier/base/chrome_async_socket_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "jingle/notifier/base/chrome_async_socket.h"
 
 #include <deque>
 #include <string>
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/message_loop.h"
-#include "base/sys_byteorder.h"
 #include "jingle/notifier/base/resolving_client_socket_factory.h"
+#include "net/base/address_list.h"
 #include "net/base/mock_cert_verifier.h"
 #include "net/base/net_errors.h"
+#include "net/base/net_util.h"
 #include "net/base/ssl_config_service.h"
 #include "net/socket/socket_test_util.h"
 #include "net/socket/ssl_client_socket.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "talk/base/sigslot.h"
+#include "talk/base/ipaddress.h"
 #include "talk/base/socketaddress.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace notifier {
 
 namespace {
 
 // Data provider that handles reads/writes for ChromeAsyncSocket.
 class AsyncSocketDataProvider : public net::SocketDataProvider {
  public:
@@ skipping 58 matching lines @@
 
  private:
   std::deque<net::MockRead> reads_;
   bool has_pending_read_;
 
   std::deque<net::MockWrite> writes_;
 
   DISALLOW_COPY_AND_ASSIGN(AsyncSocketDataProvider);
 };
 
-// Takes a 32-bit integer in host byte order and converts it to a
-// net::IPAddressNumber.
-net::IPAddressNumber Uint32ToIPAddressNumber(uint32 ip) {
-  uint32 ip_nbo = base::HostToNet32(ip);
-  const unsigned char* const ip_start =
-      reinterpret_cast<const unsigned char*>(&ip_nbo);
-  return net::IPAddressNumber(ip_start, ip_start + (sizeof ip_nbo));
-}
-
-net::AddressList SocketAddressToAddressList(
-    const talk_base::SocketAddress& address) {
-  DCHECK_NE(address.ip(), 0U);
-  return net::AddressList::CreateFromIPAddress(
-      Uint32ToIPAddressNumber(address.ip()), address.port());
-}
-
 class MockXmppClientSocketFactory : public ResolvingClientSocketFactory {
  public:
   MockXmppClientSocketFactory(
       net::ClientSocketFactory* mock_client_socket_factory,
       const net::AddressList& address_list)
           : mock_client_socket_factory_(mock_client_socket_factory),
             address_list_(address_list),
             cert_verifier_(new net::MockCertVerifier) {
   }
 
@@ skipping 19 matching lines @@
   net::SSLConfig ssl_config_;
   scoped_ptr<net::CertVerifier> cert_verifier_;
 };
 
 class ChromeAsyncSocketTest
     : public testing::Test,
       public sigslot::has_slots<> {
  protected:
   ChromeAsyncSocketTest()
       : ssl_socket_data_provider_(net::ASYNC, net::OK),
-        addr_(0xaabbccdd, 35) {}
+        addr_("example.com", 35) {}
 
   virtual ~ChromeAsyncSocketTest() {}
 
   virtual void SetUp() {
     scoped_ptr<net::MockClientSocketFactory> mock_client_socket_factory(
         new net::MockClientSocketFactory());
     mock_client_socket_factory->AddSocketDataProvider(
         &async_socket_data_provider_);
     mock_client_socket_factory->AddSSLSocketDataProvider(
         &ssl_socket_data_provider_);
 
+    net::IPAddressNumber resolved_addr;
+    EXPECT_TRUE(net::ParseIPLiteralToNumber("127.0.0.1", &resolved_addr));
+    const net::AddressList address_list =
+        net::AddressList::CreateFromIPAddress(resolved_addr, addr_.port());

[rlarocque, 2012/05/11 20:04:18]

It's probably nothing, but this has me a bit concerned.

This line creates a sockaddr that ignores the hostname part of addr_.  That
address is used (and presumably stored somewhere) on line 161.  The original
addr_ is also stored, and later used in DoOpenClosed().  

Is it a problem that the MockXmppClientSocketFactory and DoOpenClosed use
different hostnames?  Why allow this inconsistency?

[akalin, 2012/05/11 20:32:20]

sockaddrs don't store the hostname, but just the ip+port, right?  At least
Chrome's version doesn't (although I notice address_list has a canonical_name
member, it's deprecated.)

So basically DoOpenClosed uses hostname+port, and then the factory uses the
ip+port, which is faking the result of DNS resolution.

[rlarocque, 2012/05/11 20:46:16]

You're right, I was being a bit loose with the terminology.  A typical sockaddr
is just an IP address and port.  

I was concerned that the fake-resolved address might not match the results of a
DNS lookup of the hard-coded hostname.  However, if you're asserting that
"example.com" resolves to 127.0.0.1 (for the purposes of this test), then I
guess it's OK.

I'd feel slightly better about this if the hostname was "localhost", but I'm OK
with this either way.

     scoped_ptr<MockXmppClientSocketFactory> mock_xmpp_client_socket_factory(
         new MockXmppClientSocketFactory(
             mock_client_socket_factory.release(),
-            SocketAddressToAddressList(addr_)));
+            address_list));
     chrome_async_socket_.reset(
         new ChromeAsyncSocket(mock_xmpp_client_socket_factory.release(),
                               14, 20)),
 
     chrome_async_socket_->SignalConnected.connect(
         this, &ChromeAsyncSocketTest::OnConnect);
     chrome_async_socket_->SignalSSLConnected.connect(
         this, &ChromeAsyncSocketTest::OnSSLConnect);
     chrome_async_socket_->SignalClosed.connect(
         this, &ChromeAsyncSocketTest::OnClose);
@@ skipping 279 matching lines @@
   EXPECT_TRUE(chrome_async_socket_->Close());
   ExpectClosed();
   ExpectSignalSocketState(
       SignalSocketState::NoError(
           SIGNAL_CLOSE, ChromeAsyncSocket::STATE_CLOSED));
 
   EXPECT_TRUE(chrome_async_socket_->Close());
   ExpectClosed();
 }
 
-TEST_F(ChromeAsyncSocketTest, UnresolvedConnect) {
-  const talk_base::SocketAddress unresolved_addr(0, 0);
-  EXPECT_FALSE(chrome_async_socket_->Connect(unresolved_addr));
+TEST_F(ChromeAsyncSocketTest, NoHostnameConnect) {
+  talk_base::IPAddress ip_address;
+  EXPECT_TRUE(talk_base::IPFromString("127.0.0.1", &ip_address));
+  const talk_base::SocketAddress no_hostname_addr(ip_address, addr_.port());
+  EXPECT_FALSE(chrome_async_socket_->Connect(no_hostname_addr));
   ExpectErrorState(ChromeAsyncSocket::STATE_CLOSED,
                    ChromeAsyncSocket::ERROR_DNS);
 
+  EXPECT_TRUE(chrome_async_socket_->Close());
+  ExpectClosed();
+}
+
+TEST_F(ChromeAsyncSocketTest, ZeroPortConnect) {
+  const talk_base::SocketAddress zero_port_addr(addr_.hostname(), 0);
+  EXPECT_FALSE(chrome_async_socket_->Connect(zero_port_addr));
+  ExpectErrorState(ChromeAsyncSocket::STATE_CLOSED,
+                   ChromeAsyncSocket::ERROR_DNS);
+
   EXPECT_TRUE(chrome_async_socket_->Close());
   ExpectClosed();
 }
 
 TEST_F(ChromeAsyncSocketTest, DoubleConnect) {
   EXPECT_DEBUG_DEATH({
     DoOpenClosed();
 
     EXPECT_FALSE(chrome_async_socket_->Connect(addr_));
     ExpectErrorState(ChromeAsyncSocket::STATE_OPEN,
@@ skipping 580 matching lines @@
   message_loop_.RunAllPending();
 
   ExpectNoSignal();
 
   DoSSLCloseOpenedNoError();
 }
 
 }  // namespace
 
 }  // namespace notifier