LogDnsClient now rejects responses unless they contain exactly one TXT RDATA string

components/certificate_transparency/mock_log_dns_traffic.cc

Index: components/certificate_transparency/mock_log_dns_traffic.cc
diff --git a/components/certificate_transparency/mock_log_dns_traffic.cc b/components/certificate_transparency/mock_log_dns_traffic.cc
index 47922c05b2a9bdb0aa519bcb0891365720f109ae..654a5cec761e5e408bed157358070286bd14f93f 100644
--- a/components/certificate_transparency/mock_log_dns_traffic.cc
+++ b/components/certificate_transparency/mock_log_dns_traffic.cc
@@ -7,10 +7,11 @@
 #include <algorithm>
 #include <numeric>
 #include <vector>
 
 #include "base/big_endian.h"
+#include "base/strings/string_number_conversions.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"
@@ -191,34 +192,40 @@ void MockLogDnsTraffic::ExpectRequestAndTimeout(base::StringPiece qname) {
 
   // Speed up timeout tests.
   SetDnsTimeout(TestTimeouts::tiny_timeout());
 }
 
+void MockLogDnsTraffic::ExpectRequestAndResponse(
+    base::StringPiece qname,
+    const std::vector<base::StringPiece>& txt_strings) {
+  std::string answer;
+  for (base::StringPiece str : txt_strings) {
+    // The size of the string must precede it. The size must fit into 1 byte.
+    CHECK_LE(str.size(), 0xFFul);

[Ryan Sleevi, 2016/10/03 23:20:12]

nit: The 0xFFul is weird to see here. C++11 will auto-sign extend to ul/ull, so
you could just state 0xFFu.

However, CHECK'ing during test code is very much an anti-pattern that should not
be done. Tests should fail, not crash.

[Rob Percival, 2016/10/04 11:46:47]

Would you recommend instead making all of the methods in this class that
currently ASSERT/EXPECT/CHECK return bool and ASSERTing on that in the tests?

[Rob Percival, 2016/10/05 09:27:06]

Here's an implementation of that: https://codereview.chromium.org/2389993003/

+    answer.insert(answer.end(), static_cast<char>(str.size()));

[Ryan Sleevi, 2016/10/03 23:20:12]

Why not use the safe-numerics here, if you want the CHECKing behaviour?

[Rob Percival, 2016/10/04 11:46:47]

It seemed like overkill for a scenario that shouldn't occur unless someone sets
up a test with an excessively long string, in which case crashing appeared
reasonable.

[Rob Percival, 2016/10/04 15:11:05]

Done.

+    str.AppendToString(&answer);
+  }
+
+  std::vector<char> request = CreateDnsTxtRequest(qname);
+  EmplaceMockSocketData(request, CreateDnsTxtResponse(request, answer));
+}
+
 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
-  std::string answer = leaf_index.as_string();
-  answer.insert(answer.begin(), static_cast<char>(leaf_index.size()));
-
-  ExpectRequestAndResponse(qname, answer);
+    uint64_t leaf_index) {
+  ExpectRequestAndResponse(qname, { base::Uint64ToString(leaf_index) });
 }
 
 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
-  proof.insert(proof.begin(), static_cast<char>(proof.size()));
-
-  ExpectRequestAndResponse(qname, proof);
+  ExpectRequestAndResponse(qname, { proof });
 }
 
 void MockLogDnsTraffic::InitializeDnsConfig() {
   net::DnsConfig dns_config;
   // Use an invalid nameserver address. This prevents the tests accidentally
@@ -244,16 +251,10 @@ void MockLogDnsTraffic::SetDnsConfig(const net::DnsConfig& config) {
 std::unique_ptr<net::DnsClient> MockLogDnsTraffic::CreateDnsClient() {
   return net::DnsClient::CreateClientForTesting(nullptr, &socket_factory_,
                                                 base::Bind(&FakeRandInt));
 }
 
-void MockLogDnsTraffic::ExpectRequestAndResponse(base::StringPiece qname,
-                                                 base::StringPiece answer) {
-  std::vector<char> request = CreateDnsTxtRequest(qname);
-  EmplaceMockSocketData(request, CreateDnsTxtResponse(request, answer));
-}
-
 template <typename... Args>
 void MockLogDnsTraffic::EmplaceMockSocketData(Args&&... args) {
   mock_socket_data_.emplace_back(
       new MockSocketData(std::forward<Args>(args)...));
   mock_socket_data_.back()->SetReadMode(socket_read_mode_);