Minor improvements to MockLogDnsTraffic

components/certificate_transparency/mock_log_dns_traffic.cc

 // Copyright 2016 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 "components/certificate_transparency/mock_log_dns_traffic.h"
 
 #include <algorithm>
 #include <numeric>
 #include <vector>
 
 #include "base/big_endian.h"
 #include "base/sys_byteorder.h"
 #include "base/test/test_timeouts.h"
 #include "net/dns/dns_client.h"
 #include "net/dns/dns_protocol.h"
 #include "net/dns/dns_util.h"
 #include "net/socket/socket_test_util.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace certificate_transparency {
 
 namespace {
 
+// Prevents read overruns and makes a socket timeout the default behaviour.

[Ryan Sleevi, 2016/10/03 23:44:52]

How? That's perhaps the subtle part of this code, where the comment can expand
:)

// Simulates a 'hanging read'. This is used as the last mock response
// as a sentinel to prevent reading more data than expected; any caller
// who does will ultimately timeout.

(However, it's not clear why this is better than, say, just returning an
explicit net::Error)

[Rob Percival, 2016/10/05 15:42:00]

It's consistent with the other code I saw using mock sockets. However, I prefer
the idea of just returning an error, so I'll change it to do that.

+const net::MockRead kNoMoreData(net::SYNCHRONOUS, net::ERR_IO_PENDING, 2);
+
 // Necessary to expose SetDnsConfig for testing.
 class DnsChangeNotifier : public net::NetworkChangeNotifier {
  public:
   static void SetInitialDnsConfig(const net::DnsConfig& config) {
     net::NetworkChangeNotifier::SetInitialDnsConfig(config);
   }
 
   static void SetDnsConfig(const net::DnsConfig& config) {
     net::NetworkChangeNotifier::SetDnsConfig(config);
   }
 };
 
 // Always return min, to simplify testing.
 // This should result in the DNS query ID always being 0.
 int FakeRandInt(int min, int max) {
   return min;
 }
 
 std::vector<char> CreateDnsTxtRequest(base::StringPiece qname) {
   std::string encoded_qname;
-  EXPECT_TRUE(net::DNSDomainFromDot(qname, &encoded_qname));
+  DCHECK(net::DNSDomainFromDot(qname, &encoded_qname));

[Ryan Sleevi, 2016/10/03 23:44:52]

Don't DCHECK in tests. Throughout.

[Rob Percival, 2016/10/05 15:42:00]

Please see https://chromiumcodereview-hr.appspot.com/2389993003/ as a possible
resolution.

 
   // DNS query section:
   // N bytes - qname
   // 2 bytes - record type
   // 2 bytes - record class
   // Total = N + 4 bytes
   const size_t query_section_size = encoded_qname.size() + 4;
 
   std::vector<char> request(sizeof(net::dns_protocol::Header) +
                             query_section_size);
   base::BigEndianWriter writer(request.data(), request.size());
 
   // Header
   net::dns_protocol::Header header = {};
   header.flags = base::HostToNet16(net::dns_protocol::kFlagRD);
   header.qdcount = base::HostToNet16(1);
-  EXPECT_TRUE(writer.WriteBytes(&header, sizeof(header)));
+  DCHECK(writer.WriteBytes(&header, sizeof(header)));
   // Query section
-  EXPECT_TRUE(writer.WriteBytes(encoded_qname.data(), encoded_qname.size()));
-  EXPECT_TRUE(writer.WriteU16(net::dns_protocol::kTypeTXT));
-  EXPECT_TRUE(writer.WriteU16(net::dns_protocol::kClassIN));
-  EXPECT_EQ(0, writer.remaining());
+  DCHECK(writer.WriteBytes(encoded_qname.data(), encoded_qname.size()));
+  DCHECK(writer.WriteU16(net::dns_protocol::kTypeTXT));
+  DCHECK(writer.WriteU16(net::dns_protocol::kClassIN));
+  DCHECK_EQ(0, writer.remaining());
 
   return request;
 }
 
 std::vector<char> CreateDnsTxtResponse(const std::vector<char>& request,
                                        base::StringPiece answer) {
   // DNS answers section:
   // 2 bytes - qname pointer
   // 2 bytes - record type
   // 2 bytes - record class
   // 4 bytes - time-to-live
   // 2 bytes - size of answer (N)
   // N bytes - answer
   // Total = 12 + N bytes
   const size_t answers_section_size = 12 + answer.size();
   constexpr uint32_t ttl = 86400;  // seconds
 
   std::vector<char> response(request.size() + answers_section_size);
   std::copy(request.begin(), request.end(), response.begin());
   // Modify the header
   net::dns_protocol::Header* header =
       reinterpret_cast<net::dns_protocol::Header*>(response.data());
   header->ancount = base::HostToNet16(1);
   header->flags |= base::HostToNet16(net::dns_protocol::kFlagResponse);
 
   // Write the answer section
   base::BigEndianWriter writer(response.data() + request.size(),
                                response.size() - request.size());
-  EXPECT_TRUE(writer.WriteU8(0xc0));  // qname is a pointer
-  EXPECT_TRUE(writer.WriteU8(
+  DCHECK(writer.WriteU8(0xc0));  // qname is a pointer
+  DCHECK(writer.WriteU8(
       sizeof(*header)));  // address of qname (start of query section)
-  EXPECT_TRUE(writer.WriteU16(net::dns_protocol::kTypeTXT));
-  EXPECT_TRUE(writer.WriteU16(net::dns_protocol::kClassIN));
-  EXPECT_TRUE(writer.WriteU32(ttl));
-  EXPECT_TRUE(writer.WriteU16(answer.size()));
-  EXPECT_TRUE(writer.WriteBytes(answer.data(), answer.size()));
-  EXPECT_EQ(0, writer.remaining());
+  DCHECK(writer.WriteU16(net::dns_protocol::kTypeTXT));
+  DCHECK(writer.WriteU16(net::dns_protocol::kClassIN));
+  DCHECK(writer.WriteU32(ttl));
+  DCHECK(writer.WriteU16(answer.size()));
+  DCHECK(writer.WriteBytes(answer.data(), answer.size()));
+  DCHECK_EQ(0, writer.remaining());
 
   return response;
 }
 
 std::vector<char> CreateDnsErrorResponse(const std::vector<char>& request,
                                          uint8_t rcode) {
   std::vector<char> response(request);
   // Modify the header
   net::dns_protocol::Header* header =
       reinterpret_cast<net::dns_protocol::Header*>(response.data());
   header->ancount = base::HostToNet16(1);
   header->flags |= base::HostToNet16(net::dns_protocol::kFlagResponse | rcode);
 
   return response;
 }
 
 }  // namespace
 
-namespace internal {
+// A container for all of the data needed for simulating a socket.
+// This is useful because Mock{Read,Write}, SequencedSocketData and
+// MockClientSocketFactory all do not take ownership of or copy their arguments,
+// so it is necessary to manage the lifetime of those arguments. Wrapping all
+// of that up in a single class simplifies this.
+class MockLogDnsTraffic::MockSocketData {
+ public:
+  // A socket that expects one write and one read operation.
+  MockSocketData(const std::vector<char>& write, const std::vector<char>& read)
+      : expected_write_payload_(write),
+        expected_read_payload_(read),
+        expected_write_(net::SYNCHRONOUS,
+                        expected_write_payload_.data(),
+                        expected_write_payload_.size(),
+                        0),
+        expected_reads_{net::MockRead(net::ASYNC,
+                                      expected_read_payload_.data(),
+                                      expected_read_payload_.size(),
+                                      1),
+                        kNoMoreData},
+        socket_data_(expected_reads_, 2, &expected_write_, 1) {}
 
-MockSocketData::MockSocketData(const std::vector<char>& write,
-                               const std::vector<char>& read)
-    : expected_write_payload_(write),
-      expected_read_payload_(read),
-      expected_write_(net::SYNCHRONOUS,
-                      expected_write_payload_.data(),
-                      expected_write_payload_.size(),
-                      0),
-      expected_reads_{net::MockRead(net::ASYNC,
-                                    expected_read_payload_.data(),
-                                    expected_read_payload_.size(),
-                                    1),
-                      no_more_data_},
-      socket_data_(expected_reads_, 2, &expected_write_, 1) {}
+  // A socket that expects one write and a read error.
+  MockSocketData(const std::vector<char>& write, net::Error error)
+      : expected_write_payload_(write),
+        expected_write_(net::SYNCHRONOUS,
+                        expected_write_payload_.data(),
+                        expected_write_payload_.size(),
+                        0),
+        expected_reads_{net::MockRead(net::ASYNC, error, 1), kNoMoreData},
+        socket_data_(expected_reads_, 2, &expected_write_, 1) {}
 
-MockSocketData::MockSocketData(const std::vector<char>& write, int net_error)
-    : expected_write_payload_(write),
-      expected_write_(net::SYNCHRONOUS,
-                      expected_write_payload_.data(),
-                      expected_write_payload_.size(),
-                      0),
-      expected_reads_{net::MockRead(net::ASYNC, net_error, 1), no_more_data_},
-      socket_data_(expected_reads_, 2, &expected_write_, 1) {}
+  // A socket that expects one write and no response.
+  explicit MockSocketData(const std::vector<char>& write)
+      : expected_write_payload_(write),
+        expected_write_(net::SYNCHRONOUS,
+                        expected_write_payload_.data(),
+                        expected_write_payload_.size(),
+                        0),
+        expected_reads_{net::MockRead(net::SYNCHRONOUS, net::ERR_IO_PENDING, 1),
+                        kNoMoreData},
+        socket_data_(expected_reads_, 2, &expected_write_, 1) {}
 
-MockSocketData::MockSocketData(const std::vector<char>& write)
-    : expected_write_payload_(write),
-      expected_write_(net::SYNCHRONOUS,
-                      expected_write_payload_.data(),
-                      expected_write_payload_.size(),
-                      0),
-      expected_reads_{net::MockRead(net::SYNCHRONOUS, net::ERR_IO_PENDING, 1),
-                      no_more_data_},
-      socket_data_(expected_reads_, 2, &expected_write_, 1) {}
+  ~MockSocketData() {}
 
-MockSocketData::~MockSocketData() {}
+  void SetWriteMode(net::IoMode mode) { expected_write_.mode = mode; }
+  void SetReadMode(net::IoMode mode) { expected_reads_[0].mode = mode; }
 
-void MockSocketData::AddToFactory(
-    net::MockClientSocketFactory* socket_factory) {
-  socket_factory->AddSocketDataProvider(&socket_data_);
-}
+  void AddToFactory(net::MockClientSocketFactory* socket_factory) {
+    socket_factory->AddSocketDataProvider(&socket_data_);
+  }
 
-const net::MockRead MockSocketData::no_more_data_(net::SYNCHRONOUS,
-                                                  net::ERR_IO_PENDING,
-                                                  2);
+ private:
+  // This class only supports one write and one read, so just need to store one
+  // payload each.
+  const std::vector<char> expected_write_payload_;
+  const std::vector<char> expected_read_payload_;
 
-}  // namespace internal
+  // Encapsulates the data that is expected to be written to a socket.
+  net::MockWrite expected_write_;
 
-using internal::MockSocketData;
+  // Encapsulates the data/error that should be returned when reading from a
+  // socket. The second "expected" read is a sentinel that causes socket reads
+  // beyond the first to hang until they timeout. This results in better
+  // test failure messages (rather than a CHECK-fail due to a socket read
+  // overrunning the MockRead array) and behaviour more like a real socket when
+  // an unexpected second socket read occurs.
+  net::MockRead expected_reads_[2];
+
+  // Holds pointers to |expected_write_| and |expected_reads_|. This is what is
+  // added to net::MockClientSocketFactory to prepare a mock socket.
+  net::SequencedSocketData socket_data_;
+
+  DISALLOW_COPY_AND_ASSIGN(MockSocketData);
+};
 
 MockLogDnsTraffic::MockLogDnsTraffic() : socket_read_mode_(net::ASYNC) {}
 
 MockLogDnsTraffic::~MockLogDnsTraffic() {}
 
 void MockLogDnsTraffic::ExpectRequestAndErrorResponse(base::StringPiece qname,
                                                       uint8_t rcode) {
   std::vector<char> request = CreateDnsTxtRequest(qname);
   EmplaceMockSocketData(CreateDnsTxtRequest(qname),
                         CreateDnsErrorResponse(request, rcode));
 }
 
 void MockLogDnsTraffic::ExpectRequestAndSocketError(base::StringPiece qname,
-                                                    int net_error) {
-  EmplaceMockSocketData(CreateDnsTxtRequest(qname), net_error);
+                                                    net::Error error) {
+  EmplaceMockSocketData(CreateDnsTxtRequest(qname), error);
 }
 
 void MockLogDnsTraffic::ExpectRequestAndTimeout(base::StringPiece qname) {
   EmplaceMockSocketData(CreateDnsTxtRequest(qname));
 
   // Speed up timeout tests.
   SetDnsTimeout(TestTimeouts::tiny_timeout());
 }
 
 void MockLogDnsTraffic::ExpectLeafIndexRequestAndResponse(
     base::StringPiece qname,
     base::StringPiece leaf_index) {
   // Prepend size to leaf_index to create the query answer (rdata)
-  ASSERT_LE(leaf_index.size(), 0xFFul);  // size must fit into a single byte
+  DCHECK_LE(leaf_index.size(), 0xFFul);  // size must fit into a single byte
   std::string answer = leaf_index.as_string();
   answer.insert(answer.begin(), static_cast<char>(leaf_index.size()));
 
   ExpectRequestAndResponse(qname, answer);
 }
 
 void MockLogDnsTraffic::ExpectAuditProofRequestAndResponse(
     base::StringPiece qname,
     std::vector<std::string>::const_iterator audit_path_start,
     std::vector<std::string>::const_iterator audit_path_end) {
   // Join nodes in the audit path into a single string.
   std::string proof =
       std::accumulate(audit_path_start, audit_path_end, std::string());
 
   // Prepend size to proof to create the query answer (rdata)
-  ASSERT_LE(proof.size(), 0xFFul);  // size must fit into a single byte
+  DCHECK_LE(proof.size(), 0xFFul);  // size must fit into a single byte
   proof.insert(proof.begin(), static_cast<char>(proof.size()));
 
   ExpectRequestAndResponse(qname, proof);
 }
 
 void MockLogDnsTraffic::InitializeDnsConfig() {
   net::DnsConfig dns_config;
   // Use an invalid nameserver address. This prevents the tests accidentally
   // sending real DNS queries. The mock sockets don't care that the address
   // is invalid.
@@ skipping 34 matching lines @@
 }
 
 void MockLogDnsTraffic::SetDnsTimeout(const base::TimeDelta& timeout) {
   net::DnsConfig dns_config;
   DnsChangeNotifier::GetDnsConfig(&dns_config);
   dns_config.timeout = timeout;
   DnsChangeNotifier::SetDnsConfig(dns_config);
 }
 
 }  // namespace certificate_transparency