[net/dns] Don't abandon a DnsUDPAttempt when the response does not match the query.

net/dns/dns_transaction_unittest.cc

Index: net/dns/dns_transaction_unittest.cc
diff --git a/net/dns/dns_transaction_unittest.cc b/net/dns/dns_transaction_unittest.cc
index 5e61f273ecffa64e35bc1d73c062ea4794465edf..29fe69c17c4174c79f0372a91b2a379b3f3c9cf6 100644
--- a/net/dns/dns_transaction_unittest.cc
+++ b/net/dns/dns_transaction_unittest.cc
@@ -31,6 +31,74 @@ std::string DomainFromDot(const base::StringPiece& dotted) {
   return out;
 }
 
+// A SocketDataProvider builder for MockUDPClientSocket. Each socket used by a
+// DnsTransaction expects only one write and zero or more reads.
+class DnsSocketData {
+ public:
+  // The ctor takes parameters for the DnsQuery.
+  DnsSocketData(const char* dotted_name, uint16 qtype, uint16 id, IoMode mode)
+      : query_(new DnsQuery(id, DomainFromDot(dotted_name), qtype)),
+        write_(mode, query_->io_buffer()->data(), query_->io_buffer()->size()) {
+  }
+  ~DnsSocketData() {}
+
+  // All responses must be added before GetProvider.
+  void AddResponseData(const char* data, size_t length, IoMode mode) {
+    CHECK(!provider_.get());
+    AddResponse(make_scoped_ptr(new DnsResponse(data, length, 0)), mode);
+  }
+
+  void AddRcode(int rcode, IoMode mode) {

[mmenke, 2012/08/29 16:23:57]

This and AddResponse should have comments - particularly this one.

+    scoped_ptr<DnsResponse> response(
+        new DnsResponse(query_->io_buffer()->data(),
+                        query_->io_buffer()->size(),
+                        0));
+    dns_protocol::Header* header =
+        reinterpret_cast<dns_protocol::Header*>(response->io_buffer()->data());
+    header->flags |= base::HostToNet16(dns_protocol::kFlagResponse | rcode);
+    AddResponse(response.Pass(), mode);
+  }
+
+  void AddResponse(scoped_ptr<DnsResponse> response, IoMode mode) {

[mmenke, 2012/08/29 16:23:57]

nit:  Suggest you put this before AddRcode, since that's the same order the
corresponding functions appear in below.

+    CHECK(!provider_.get());
+    reads_.push_back(MockRead(mode,
+                              response->io_buffer()->data(),
+                              response->io_buffer()->size()));
+    responses_.push_back(response.release());
+  }
+
+  SocketDataProvider* GetProvider() {
+    if (provider_.get())
+      return provider_.get();
+    if (reads_.empty()) {
+      // Timeout.
+      provider_.reset(new DelayedSocketData(2, NULL, 0, &write_, 1));
+    } else {
+      provider_.reset(new DelayedSocketData(1, &reads_[0], reads_.size(),
+                                            &write_, 1));
+    }
+    return provider_.get();
+  }
+
+  uint16 query_id() const {
+    return query_->id();
+  }
+
+  bool got_written() const {

[mmenke, 2012/08/29 16:23:57]

nit:  Suggest was_written(), or maybe query_was_written().

+    CHECK(provider_.get());
+    return provider_->write_index() > 0;
+  }
+
+ private:
+  scoped_ptr<DnsQuery> query_;
+  ScopedVector<DnsResponse> responses_;
+  MockWrite write_;
+  std::vector<MockRead> reads_;
+  scoped_ptr<DelayedSocketData> provider_;
+
+  DISALLOW_COPY_AND_ASSIGN(DnsSocketData);
+};
+
 class TestSocketFactory;
 
 // A variant of MockUDPClientSocket which notifies the factory OnConnect.
@@ -45,6 +113,8 @@ class TestUDPClientSocket : public MockUDPClientSocket {
   virtual int Connect(const IPEndPoint& endpoint) OVERRIDE;
  private:

[mmenke, 2012/08/29 16:23:57]

While you're here, there should be a blank line above private.

   TestSocketFactory* factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(TestUDPClientSocket);
 };
 
 // Creates TestUDPClientSockets and keeps endpoints reported via OnConnect.
@@ -71,6 +141,8 @@ class TestSocketFactory : public MockClientSocketFactory {
   }
 
   std::vector<IPEndPoint> remote_endpoints;
+
+  DISALLOW_COPY_AND_ASSIGN(TestSocketFactory);
 };
 
 int TestUDPClientSocket::Connect(const IPEndPoint& endpoint) {
@@ -139,8 +211,12 @@ class TransactionHelper {
       return;
     }
 
+    if (quit_in_callback_)

[mmenke, 2012/08/29 16:23:57]

Please add a comment that this needs to be done before the asserts below.

+      MessageLoop::current()->Quit();
+
     if (expected_answer_count_ >= 0) {
-      EXPECT_EQ(OK, rv);
+      ASSERT_EQ(OK, rv);
+      ASSERT_TRUE(response != NULL);
       EXPECT_EQ(static_cast<unsigned>(expected_answer_count_),
                 response->answer_count());
       EXPECT_EQ(qtype_, response->qtype());
@@ -153,9 +229,6 @@ class TransactionHelper {
     } else {
       EXPECT_EQ(expected_answer_count_, rv);
     }
-
-    if (quit_in_callback_)
-      MessageLoop::current()->Quit();
   }
 
   bool has_completed() const {
@@ -220,37 +293,29 @@ class DnsTransactionTest : public testing::Test {
     transaction_factory_ = DnsTransactionFactory::CreateFactory(session_.get());
   }
 
-  // Each socket used by a DnsTransaction expects only one write and zero or one
-  // reads.
+  void AddSocketData(scoped_ptr<DnsSocketData> data) {
+    transaction_ids_.push_back(data->query_id());
+    socket_factory_->AddSocketDataProvider(data->GetProvider());
+    socket_data_.push_back(data.release());
+  }
 
   // Add expected query for |dotted_name| and |qtype| with |id| and response
   // taken verbatim from |data| of |data_length| bytes. The transaction id in
   // |data| should equal |id|, unless testing mismatched response.
-  void AddResponse(const std::string& dotted_name,
+  void AddResponse(const char* dotted_name,

[mmenke, 2012/08/29 16:23:57]

While you're here, could you rename this?  The comment is accurate, but the name
is misleading.  AddQueryAndResponse, maybe?

                    uint16 qtype,
                    uint16 id,
-                   const char* data,
-                   size_t data_length,
+                   const char* response_data,
+                   size_t response_length,
                    IoMode mode) {
     CHECK(socket_factory_.get());
-    DnsQuery* query = new DnsQuery(id, DomainFromDot(dotted_name), qtype);
-    queries_.push_back(query);
-
-    // The response is only used to hold the IOBuffer.
-    DnsResponse* response = new DnsResponse(data, data_length, 0);
-    responses_.push_back(response);
-
-    writes_.push_back(MockWrite(mode,
-                                query->io_buffer()->data(),
-                                query->io_buffer()->size()));
-    reads_.push_back(MockRead(mode,
-                              response->io_buffer()->data(),
-                              data_length));
-
-    transaction_ids_.push_back(id);
+    scoped_ptr<DnsSocketData> data(
+        new DnsSocketData(dotted_name, qtype, id, mode));
+    data->AddResponseData(response_data, response_length, mode);
+    AddSocketData(data.Pass());
   }
 
-  void AddAsyncResponse(const std::string& dotted_name,
+  void AddAsyncResponse(const char* dotted_name,
                         uint16 qtype,
                         uint16 id,
                         const char* data,
@@ -258,19 +323,21 @@ class DnsTransactionTest : public testing::Test {
     AddResponse(dotted_name, qtype, id, data, data_length, ASYNC);
   }
 
+  void AddSyncResponse(const char* dotted_name,
+                       uint16 qtype,
+                       uint16 id,
+                       const char* data,
+                       size_t data_length) {
+    AddResponse(dotted_name, qtype, id, data, data_length, SYNCHRONOUS);
+  }
+
   // Add expected query of |dotted_name| and |qtype| and no response.
   void AddTimeout(const char* dotted_name, uint16 qtype) {
     CHECK(socket_factory_.get());
     uint16 id = base::RandInt(0, kuint16max);
-    DnsQuery* query = new DnsQuery(id, DomainFromDot(dotted_name), qtype);
-    queries_.push_back(query);
-
-    writes_.push_back(MockWrite(ASYNC,
-                                query->io_buffer()->data(),
-                                query->io_buffer()->size()));
-    // Empty MockRead indicates no data.
-    reads_.push_back(MockRead());
-    transaction_ids_.push_back(id);
+    scoped_ptr<DnsSocketData> data(
+        new DnsSocketData(dotted_name, qtype, id, ASYNC));
+    AddSocketData(data.Pass());
   }
 
   // Add expected query of |dotted_name| and |qtype| and response with no answer
@@ -279,46 +346,18 @@ class DnsTransactionTest : public testing::Test {
     CHECK(socket_factory_.get());
     CHECK_NE(dns_protocol::kRcodeNOERROR, rcode);
     uint16 id = base::RandInt(0, kuint16max);
-    DnsQuery* query = new DnsQuery(id, DomainFromDot(dotted_name), qtype);
-    queries_.push_back(query);
-
-    DnsResponse* response = new DnsResponse(query->io_buffer()->data(),
-                                            query->io_buffer()->size(),
-                                            0);
-    dns_protocol::Header* header =
-        reinterpret_cast<dns_protocol::Header*>(response->io_buffer()->data());
-    header->flags |= base::HostToNet16(dns_protocol::kFlagResponse | rcode);
-    responses_.push_back(response);
-
-    writes_.push_back(MockWrite(mode,
-                                query->io_buffer()->data(),
-                                query->io_buffer()->size()));
-    reads_.push_back(MockRead(mode,
-                              response->io_buffer()->data(),
-                              query->io_buffer()->size()));
-    transaction_ids_.push_back(id);
+    scoped_ptr<DnsSocketData> data(
+        new DnsSocketData(dotted_name, qtype, id, mode));
+    data->AddRcode(rcode, mode);
+    AddSocketData(data.Pass());
   }
 
   void AddAsyncRcode(const char* dotted_name, uint16 qtype, int rcode) {
     AddRcode(dotted_name, qtype, rcode, ASYNC);
   }
 
-  // Call after all Add* calls to prepare data for |socket_factory_|.
-  // This separation is necessary because the |reads_| and |writes_| vectors
-  // could reallocate their data during those calls.
-  void PrepareSockets() {
-    CHECK_EQ(writes_.size(), reads_.size());
-    for (size_t i = 0; i < writes_.size(); ++i) {
-      DelayedSocketData* data;
-      if (reads_[i].data) {
-        data = new DelayedSocketData(1, &reads_[i], 1, &writes_[i], 1);
-      } else {
-        // Timeout.
-        data = new DelayedSocketData(2, NULL, 0, &writes_[i], 1);
-      }
-      socket_data_.push_back(data);
-      socket_factory_->AddSocketDataProvider(data);
-    }
+  void AddSyncRcode(const char* dotted_name, uint16 qtype, int rcode) {
+    AddRcode(dotted_name, qtype, rcode, SYNCHRONOUS);
   }
 
   // Checks if the sockets were connected in the order matching the indices in
@@ -344,7 +383,7 @@ class DnsTransactionTest : public testing::Test {
   void TearDown() OVERRIDE {
     // Check that all socket data was at least written to.
     for (size_t i = 0; i < socket_data_.size(); ++i) {
-      EXPECT_GT(socket_data_[i]->write_index(), 0u);
+      EXPECT_TRUE(socket_data_[i]->got_written()) << i;
     }
   }
 
@@ -360,16 +399,7 @@ class DnsTransactionTest : public testing::Test {
 
   DnsConfig config_;
 
-  // Holders for the buffers behind MockRead/MockWrites (they do not own them).
-  ScopedVector<DnsQuery> queries_;
-  ScopedVector<DnsResponse> responses_;
-
-  // Holders for MockRead/MockWrites (SocketDataProvider does not own it).
-  std::vector<MockRead> reads_;
-  std::vector<MockWrite> writes_;
-
-  // Holder for the socket data (MockClientSocketFactory does not own it).
-  ScopedVector<DelayedSocketData> socket_data_;
+  ScopedVector<DnsSocketData> socket_data_;
 
   std::deque<int> transaction_ids_;
   scoped_ptr<TestSocketFactory> socket_factory_;
@@ -383,7 +413,6 @@ TEST_F(DnsTransactionTest, Lookup) {
                    0 /* id */,
                    reinterpret_cast<const char*>(kT0ResponseDatagram),
                    arraysize(kT0ResponseDatagram));
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -404,7 +433,6 @@ TEST_F(DnsTransactionTest, ConcurrentLookup) {
                    1 /* id */,
                    reinterpret_cast<const char*>(kT1ResponseDatagram),
                    arraysize(kT1ResponseDatagram));
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -432,7 +460,6 @@ TEST_F(DnsTransactionTest, CancelLookup) {
                    1 /* id */,
                    reinterpret_cast<const char*>(kT1ResponseDatagram),
                    arraysize(kT1ResponseDatagram));
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -457,7 +484,6 @@ TEST_F(DnsTransactionTest, DestroyFactory) {
                    0 /* id */,
                    reinterpret_cast<const char*>(kT0ResponseDatagram),
                    arraysize(kT0ResponseDatagram));
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -478,7 +504,6 @@ TEST_F(DnsTransactionTest, CancelFromCallback) {
                    0 /* id */,
                    reinterpret_cast<const char*>(kT0ResponseDatagram),
                    arraysize(kT0ResponseDatagram));
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -487,9 +512,50 @@ TEST_F(DnsTransactionTest, CancelFromCallback) {
   EXPECT_TRUE(helper0.Run(transaction_factory_.get()));
 }
 
+TEST_F(DnsTransactionTest, MismatchedResponseSync) {

[mmenke, 2012/08/29 16:23:57]

I'd also like a test where we get an ERR_DNS_MALFORMED_RESPONSE

[szym, 2012/08/29 21:37:26]

That was not possible in this patch. If the transaction could't make any further
attempts it would wait until the last attempt timed out. See
dns_transaction.cc:520

If we fail early, we will still be vulnerable to unrelated packets, just
slightly less if retries are allowed. If we wait until timeout, we might be
fruitlessly stalling the whole HostResolver::Request. I'd rather err on the
optimistic site for now.

+  config_.attempts = 2;
+  config_.timeout = TestTimeouts::tiny_timeout();
+  ConfigureFactory();
+
+  // Attempt receives mismatched response followed by valid response.
+  scoped_ptr<DnsSocketData> data(
+      new DnsSocketData(kT0HostName, kT0Qtype, 0, SYNCHRONOUS));
+  data->AddResponseData(reinterpret_cast<const char*>(kT1ResponseDatagram),
+                        arraysize(kT1ResponseDatagram), SYNCHRONOUS);
+  data->AddResponseData(reinterpret_cast<const char*>(kT0ResponseDatagram),
+                        arraysize(kT0ResponseDatagram), SYNCHRONOUS);
+  AddSocketData(data.Pass());
+
+  TransactionHelper helper0(kT0HostName,
+                            kT0Qtype,
+                            kT0RecordCount);
+  EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
+}
+
+TEST_F(DnsTransactionTest, MismatchedResponseAsync) {
+  config_.attempts = 2;
+  config_.timeout = TestTimeouts::tiny_timeout();
+  ConfigureFactory();
+
+  // First attempt receives mismatched response followed by valid response.
+  // Second attempt times out.
+  scoped_ptr<DnsSocketData> data(
+      new DnsSocketData(kT0HostName, kT0Qtype, 0, ASYNC));
+  data->AddResponseData(reinterpret_cast<const char*>(kT1ResponseDatagram),
+                        arraysize(kT1ResponseDatagram), ASYNC);
+  data->AddResponseData(reinterpret_cast<const char*>(kT0ResponseDatagram),
+                        arraysize(kT0ResponseDatagram), ASYNC);
+  AddSocketData(data.Pass());
+  AddTimeout(kT0HostName, kT0Qtype);
+
+  TransactionHelper helper0(kT0HostName,
+                            kT0Qtype,
+                            kT0RecordCount);
+  EXPECT_TRUE(helper0.RunUntilDone(transaction_factory_.get()));
+}
+
 TEST_F(DnsTransactionTest, ServerFail) {
   AddAsyncRcode(kT0HostName, kT0Qtype, dns_protocol::kRcodeSERVFAIL);
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -499,7 +565,6 @@ TEST_F(DnsTransactionTest, ServerFail) {
 
 TEST_F(DnsTransactionTest, NoDomain) {
   AddAsyncRcode(kT0HostName, kT0Qtype, dns_protocol::kRcodeNXDOMAIN);
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -516,7 +581,6 @@ TEST_F(DnsTransactionTest, Timeout) {
   AddTimeout(kT0HostName, kT0Qtype);
   AddTimeout(kT0HostName, kT0Qtype);
   AddTimeout(kT0HostName, kT0Qtype);
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -544,7 +608,6 @@ TEST_F(DnsTransactionTest, ServerFallbackAndRotate) {
   // Responses for second request.
   AddAsyncRcode(kT1HostName, kT1Qtype, dns_protocol::kRcodeSERVFAIL);
   AddAsyncRcode(kT1HostName, kT1Qtype, dns_protocol::kRcodeNXDOMAIN);
-  PrepareSockets();
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -576,7 +639,6 @@ TEST_F(DnsTransactionTest, SuffixSearchAboveNdots) {
   AddAsyncRcode("x.y.z.a", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
   AddAsyncRcode("x.y.z.b", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
   AddAsyncRcode("x.y.z.c", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
-  PrepareSockets();
 
   TransactionHelper helper0("x.y.z",
                             dns_protocol::kTypeA,
@@ -607,7 +669,6 @@ TEST_F(DnsTransactionTest, SuffixSearchBelowNdots) {
   AddAsyncRcode("x.c", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
   // Responses for third transaction.
   AddAsyncRcode("x", dns_protocol::kTypeAAAA, dns_protocol::kRcodeNXDOMAIN);
-  PrepareSockets();
 
   TransactionHelper helper0("x.y",
                             dns_protocol::kTypeA,
@@ -633,7 +694,6 @@ TEST_F(DnsTransactionTest, SuffixSearchBelowNdots) {
 TEST_F(DnsTransactionTest, EmptySuffixSearch) {
   // Responses for first transaction.
   AddAsyncRcode("x", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
-  PrepareSockets();
 
   // A fully-qualified name.
   TransactionHelper helper0("x.",
@@ -666,7 +726,6 @@ TEST_F(DnsTransactionTest, DontAppendToMultiLabelName) {
   AddAsyncRcode("x.a", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
   AddAsyncRcode("x.b", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
   AddAsyncRcode("x.c", dns_protocol::kTypeA, dns_protocol::kRcodeNXDOMAIN);
-  PrepareSockets();
 
   TransactionHelper helper0("x.y.z",
                             dns_protocol::kTypeA,
@@ -713,7 +772,6 @@ TEST_F(DnsTransactionTest, SuffixSearchStop) {
                    0 /* id */,
                    reinterpret_cast<const char*>(kResponseNoData),
                    arraysize(kResponseNoData));
-  PrepareSockets();
 
   TransactionHelper helper0("x.y.z", dns_protocol::kTypeA, 0 /* answers */);
 
@@ -725,13 +783,11 @@ TEST_F(DnsTransactionTest, SyncFirstQuery) {
   config_.search.push_back("ccs.neu.edu");
   ConfigureFactory();
 
-  AddResponse(kT0HostName,
-              kT0Qtype,
-              0 /* id */,
-              reinterpret_cast<const char*>(kT0ResponseDatagram),
-              arraysize(kT0ResponseDatagram),
-              SYNCHRONOUS);
-  PrepareSockets();
+  AddSyncResponse(kT0HostName,
+                  kT0Qtype,
+                  0 /* id */,
+                  reinterpret_cast<const char*>(kT0ResponseDatagram),
+                  arraysize(kT0ResponseDatagram));
 
   TransactionHelper helper0(kT0HostName,
                             kT0Qtype,
@@ -744,17 +800,14 @@ TEST_F(DnsTransactionTest, SyncFirstQueryWithSearch) {
   config_.search.push_back("ccs.neu.edu");
   ConfigureFactory();
 
-  AddRcode("www.lab.ccs.neu.edu",
-           kT2Qtype,
-           dns_protocol::kRcodeNXDOMAIN,
-           SYNCHRONOUS);
-  AddResponse(kT2HostName,  // "www.ccs.neu.edu"
-              kT2Qtype,
-              2 /* id */,
-              reinterpret_cast<const char*>(kT2ResponseDatagram),
-              arraysize(kT2ResponseDatagram),
-              ASYNC);
-  PrepareSockets();
+  AddSyncRcode("www.lab.ccs.neu.edu",
+               kT2Qtype,
+               dns_protocol::kRcodeNXDOMAIN);
+  AddAsyncResponse(kT2HostName,  // "www.ccs.neu.edu"
+                   kT2Qtype,
+                   2 /* id */,
+                   reinterpret_cast<const char*>(kT2ResponseDatagram),
+                   arraysize(kT2ResponseDatagram));
 
   TransactionHelper helper0("www",
                             kT2Qtype,
@@ -767,17 +820,14 @@ TEST_F(DnsTransactionTest, SyncSearchQuery) {
   config_.search.push_back("ccs.neu.edu");
   ConfigureFactory();
 
-  AddRcode("www.lab.ccs.neu.edu",
-           dns_protocol::kTypeA,
-           dns_protocol::kRcodeNXDOMAIN,
-           ASYNC);
-  AddResponse(kT2HostName,
-              kT2Qtype,
-              2 /* id */,
-              reinterpret_cast<const char*>(kT2ResponseDatagram),
-              arraysize(kT2ResponseDatagram),
-              SYNCHRONOUS);
-  PrepareSockets();
+  AddAsyncRcode("www.lab.ccs.neu.edu",
+                dns_protocol::kTypeA,
+               dns_protocol::kRcodeNXDOMAIN);
+  AddSyncResponse(kT2HostName,
+                  kT2Qtype,
+                  2 /* id */,
+                  reinterpret_cast<const char*>(kT2ResponseDatagram),
+                  arraysize(kT2ResponseDatagram));
 
   TransactionHelper helper0("www",
                             kT2Qtype,