Improve documentation and readability of CTLogVerifier tests

net/cert/ct_log_verifier_unittest.cc

Index: net/cert/ct_log_verifier_unittest.cc
diff --git a/net/cert/ct_log_verifier_unittest.cc b/net/cert/ct_log_verifier_unittest.cc
index 754d3e39073c50f89c3c83b1d3d623a7ef782659..ff911d6a14b729f9bd4f0a356fed465b9d86645b 100644
--- a/net/cert/ct_log_verifier_unittest.cc
+++ b/net/cert/ct_log_verifier_unittest.cc
@@ -41,8 +41,8 @@ uint64_t CalculateNearestPowerOfTwo(uint64_t n) {
 
 // A single hash node.
 struct TestVector {
-  const char* const str;
-  size_t length_bytes;
+  const char* const str;  // hex string
+  size_t length_bytes;    // number of bytes represented by |str|
 };
 
 // A single consistency proof. Contains the old and new tree sizes
@@ -61,8 +61,8 @@ struct ProofTestVector {
 const TestVector kSHA256EmptyTreeHash = {
     "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", 32};
 
-// Node hashes for a sample tree of size 8 (each element in this array is
-// a node hash, not leaf data; order represents order of the nodes in the tree).
+// Incremental roots from building the sample tree of size 8 leaf-by-leaf.
+// The first entry is the root at size 0, the last is the root at size 8.
 const TestVector kSHA256Roots[8] = {
     {"6e340b9cffb37a989ca544e6bb780a2c78901d3fb33738768511a30617afa01d", 32},
     {"fac54203e7cc696cf0dfcb42c92a1d9dbaf70ad9e621f4bd8d98662f00e3c125", 32},
@@ -73,8 +73,7 @@ const TestVector kSHA256Roots[8] = {
     {"ddb89be403809e325750d3d263cd78929c2942b7942a34b77e122c9594a74c8c", 32},
     {"5dc9da79a70659a9ad559cb701ded9a2ab9d823aad2f4960cfe370eff4604328", 32}};
 
-// A collection of consistency proofs between various sub-trees of the tree
-// defined by |kSHA256Roots|.
+// A collection of consistency proofs between various nodes of the sample tree.
 const ProofTestVector kSHA256Proofs[4] = {
     // Empty consistency proof between trees of the same size (1).
     {1, 1, 0, {{"", 0}, {"", 0}, {"", 0}}},
@@ -115,9 +114,14 @@ std::string HexToBytes(const char* hex_data, size_t hex_data_length) {
   return result;
 }
 
+std::string HexToBytes(const TestVector& x) {
+  std::string bytes = HexToBytes(x.str, x.length_bytes * 2);
+  CHECK_EQ(x.length_bytes, bytes.size());
+  return bytes;
+}
+
 std::string GetEmptyTreeHash() {
-  return HexToBytes(kSHA256EmptyTreeHash.str,
-                    kSHA256EmptyTreeHash.length_bytes);

[Ryan Sleevi, 2016/07/26 18:25:14]

Wait, so this whole time, the empty tree hash was truncated by 32-bytes, and
nothing failed?

[Eran Messeri, 2016/07/26 18:32:08]

Turns out it wasn't, because of a bug in the HexToBytes variant that takes a
(const char*, size_t):
It constructed  hex_data_input of type std::string using only half the hex bytes
(truncating it), but never used the hex_data_input string, passing the input
const char* instead to base::HexStringToBytes (which presumably constructed a
std::string with the entire hex string as it is null-terminated).

 

[Ryan Sleevi, 2016/07/26 18:36:24]

So why does this CL leave the bug in place then? (Line 112)

My gripe is that from a reviewers point, there's something that very glaringly
seems a behaviour change (the *2), without documenting in the CL description or
reviewer notes. Even if it's benign, that's totally something that *should* have
been documented, to avoid the first set of questions. Arguably, however, if
we're saying it's a bug, we shouldn't add a workaround for the bug (line 118)
and not even fix the bug (line 112) - pick one or the other. Either we should
remove the length variant from HexToBytes or properly use hex_data_input

[Rob Percival, 2016/08/25 16:23:34]

I've broken out a fix for this into https://codereview.chromium.org/2275353002.
I'll rebase this change on top of that, so that this is pretty much just a
documentation change.

+  return HexToBytes(kSHA256EmptyTreeHash);
 }
 
 // Creates a ct::MerkleConsistencyProof and returns the result of
@@ -379,18 +383,18 @@ TEST_F(CTLogVerifierTest, VerifiesValidConsistencyProofs) {
 
   // Known good proofs.
   for (size_t i = 0; i < arraysize(kSHA256Proofs); ++i) {
+    SCOPED_TRACE(i);
     proof.clear();
     for (size_t j = 0; j < kSHA256Proofs[i].proof_length; ++j) {
       const TestVector& v = kSHA256Proofs[i].proof[j];
-      proof.push_back(HexToBytes(v.str, v.length_bytes));
+      proof.push_back(HexToBytes(v));
     }
     const uint64_t snapshot1 = kSHA256Proofs[i].snapshot1;
     const uint64_t snapshot2 = kSHA256Proofs[i].snapshot2;
     const TestVector& old_root = kSHA256Roots[snapshot1 - 1];
     const TestVector& new_root = kSHA256Roots[snapshot2 - 1];
-    VerifierConsistencyCheck(
-        snapshot1, snapshot2, HexToBytes(old_root.str, old_root.length_bytes),
-        HexToBytes(new_root.str, new_root.length_bytes), proof);
+    VerifierConsistencyCheck(snapshot1, snapshot2, HexToBytes(old_root),
+                             HexToBytes(new_root), proof);
   }
 }
 
@@ -402,10 +406,7 @@ const char kLeafPrefix[] = {'\x00'};
 // code.
 class TreeHasher {
  public:
-  static std::string HashEmpty() {
-    return HexToBytes(kSHA256EmptyTreeHash.str,
-                      kSHA256EmptyTreeHash.length_bytes);
-  }
+  static std::string HashEmpty() { return HexToBytes(kSHA256EmptyTreeHash); }
 
   static std::string HashLeaf(const std::string& leaf) {
     SHA256HashValue sha256;