Removes TestVector from ct_log_verifier_unittest

net/cert/ct_log_verifier_unittest.cc

 // Copyright 2013 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 "net/cert/ct_log_verifier.h"
 
 #include <stdint.h>
 
 #include <memory>
 #include <string>
@@ skipping 19 matching lines @@
 uint64_t CalculateNearestPowerOfTwo(uint64_t n) {
   DCHECK_GT(n, 1u);
 
   uint64_t ret = UINT64_C(1) << 63;
   while (ret >= n)
     ret >>= 1;
 
   return ret;
 }
 
-// The following structures, TestVector and ProofTestVector are used for
-// definining test proofs later on.
-
-// A single hash node.
-struct TestVector {
-  const char* const str;
-  size_t length_bytes;
-};
-
 // A single consistency proof. Contains the old and new tree sizes
 // (snapshot1 and snapshot2), the length of the proof (proof_length) and
 // at most 3 proof nodes (all test proofs will be for a tree of size 8).
 struct ProofTestVector {
   uint64_t snapshot1;
   uint64_t snapshot2;
   size_t proof_length;
-  TestVector proof[3];
+  const char* const proof[3];
 };
 
 // All test data replicated from
 // https://github.com/google/certificate-transparency/blob/c41b090ecc14ddd6b3531dc7e5ce36b21e253fdd/cpp/merkletree/merkle_tree_test.cc
 // A hash of the empty string.
-const TestVector kSHA256EmptyTreeHash = {
-    "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855", 32};
+const char* const kSHA256EmptyTreeHash =
+    "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855";
 
 // 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).
-const TestVector kSHA256Roots[8] = {
-    {"6e340b9cffb37a989ca544e6bb780a2c78901d3fb33738768511a30617afa01d", 32},
-    {"fac54203e7cc696cf0dfcb42c92a1d9dbaf70ad9e621f4bd8d98662f00e3c125", 32},
-    {"aeb6bcfe274b70a14fb067a5e5578264db0fa9b51af5e0ba159158f329e06e77", 32},
-    {"d37ee418976dd95753c1c73862b9398fa2a2cf9b4ff0fdfe8b30cd95209614b7", 32},
-    {"4e3bbb1f7b478dcfe71fb631631519a3bca12c9aefca1612bfce4c13a86264d4", 32},
-    {"76e67dadbcdf1e10e1b74ddc608abd2f98dfb16fbce75277b5232a127f2087ef", 32},
-    {"ddb89be403809e325750d3d263cd78929c2942b7942a34b77e122c9594a74c8c", 32},
-    {"5dc9da79a70659a9ad559cb701ded9a2ab9d823aad2f4960cfe370eff4604328", 32}};
+const char* const kSHA256Roots[8] = {
+    "6e340b9cffb37a989ca544e6bb780a2c78901d3fb33738768511a30617afa01d",
+    "fac54203e7cc696cf0dfcb42c92a1d9dbaf70ad9e621f4bd8d98662f00e3c125",
+    "aeb6bcfe274b70a14fb067a5e5578264db0fa9b51af5e0ba159158f329e06e77",
+    "d37ee418976dd95753c1c73862b9398fa2a2cf9b4ff0fdfe8b30cd95209614b7",
+    "4e3bbb1f7b478dcfe71fb631631519a3bca12c9aefca1612bfce4c13a86264d4",
+    "76e67dadbcdf1e10e1b74ddc608abd2f98dfb16fbce75277b5232a127f2087ef",
+    "ddb89be403809e325750d3d263cd78929c2942b7942a34b77e122c9594a74c8c",
+    "5dc9da79a70659a9ad559cb701ded9a2ab9d823aad2f4960cfe370eff4604328"};
 
 // A collection of consistency proofs between various sub-trees of the tree
 // defined by |kSHA256Roots|.
 const ProofTestVector kSHA256Proofs[4] = {
     // Empty consistency proof between trees of the same size (1).
-    {1, 1, 0, {{"", 0}, {"", 0}, {"", 0}}},
+    {1, 1, 0, {"", "", ""}},
     // Consistency proof between tree of size 1 and tree of size 8, with 3
     // nodes in the proof.
     {1,
      8,
      3,
-     {{"96a296d224f285c67bee93c30f8a309157f0daa35dc5b87e410b78630a09cfc7", 32},
-      {"5f083f0a1a33ca076a95279832580db3e0ef4584bdff1f54c8a360f50de3031e", 32},
-      {"6b47aaf29ee3c2af9af889bc1fb9254dabd31177f16232dd6aab035ca39bf6e4",
-       32}}},
+     {"96a296d224f285c67bee93c30f8a309157f0daa35dc5b87e410b78630a09cfc7",
+      "5f083f0a1a33ca076a95279832580db3e0ef4584bdff1f54c8a360f50de3031e",
+      "6b47aaf29ee3c2af9af889bc1fb9254dabd31177f16232dd6aab035ca39bf6e4"}},
     // Consistency proof between tree of size 6 and tree of size 8, with 3
     // nodes in the proof.
     {6,
      8,
      3,
-     {{"0ebc5d3437fbe2db158b9f126a1d118e308181031d0a949f8dededebc558ef6a", 32},
-      {"ca854ea128ed050b41b35ffc1b87b8eb2bde461e9e3b5596ece6b9d5975a0ae0", 32},
-      {"d37ee418976dd95753c1c73862b9398fa2a2cf9b4ff0fdfe8b30cd95209614b7",
-       32}}},
+     {"0ebc5d3437fbe2db158b9f126a1d118e308181031d0a949f8dededebc558ef6a",
+      "ca854ea128ed050b41b35ffc1b87b8eb2bde461e9e3b5596ece6b9d5975a0ae0",
+      "d37ee418976dd95753c1c73862b9398fa2a2cf9b4ff0fdfe8b30cd95209614b7"}},
     // Consistency proof between tree of size 2 and tree of size 5, with 2
     // nodes in the proof.
     {2,
      5,
      2,
-     {{"5f083f0a1a33ca076a95279832580db3e0ef4584bdff1f54c8a360f50de3031e", 32},
-      {"bc1a0643b12e4d2d7c77918f44e0f4f79a838b6cf9ec5b5c283e1f4d88599e6b", 32},
-      {"", 0}}}};
+     {"5f083f0a1a33ca076a95279832580db3e0ef4584bdff1f54c8a360f50de3031e",
+      "bc1a0643b12e4d2d7c77918f44e0f4f79a838b6cf9ec5b5c283e1f4d88599e6b", ""}}};
 
 // Decodes a hexadecimal string into the binary data it represents.
-std::string HexToBytes(const char* hex_data, size_t hex_data_length) {
+std::string HexToBytes(const std::string& hex_data) {

[Ryan Sleevi, 2016/08/25 18:32:11]

DESIGN: You're (almost) always passing string constants to this function, but
forcing them to be copied into std::string()'s, which adds overhead.

A StringPiece would avoid the copy, but because the API contract itself is a
constant string with no embedded NULLs, is there any reason not to do

std::string HexToBytes(const char* hex_data)

[Rob Percival, 2016/08/25 19:35:13]

base::HexStringToBytes takes a const std::string&, so the overhead has to be
paid anyway.

[Ryan Sleevi, 2016/08/25 20:15:20]

So then, is this a good API to be using? Should you just store the raw byte
literals then, to avoid the convenience API that's adding measurable overhead to
test execution?

That is,
\x00\x01\x02 etc

[Rob Percival, 2016/08/25 20:26:40]

As I said in a separate comment, I'm in the process of making that change now
(in a follow-up CL).

   std::vector<uint8_t> output;
   std::string result;
-  std::string hex_data_input(hex_data, hex_data_length);
   if (base::HexStringToBytes(hex_data, &output))
-    result.assign(reinterpret_cast<const char*>(&output[0]), output.size());
+    result.assign(output.begin(), output.end());
   return result;
 }
 
-std::string GetEmptyTreeHash() {
-  return HexToBytes(kSHA256EmptyTreeHash.str,
-                    kSHA256EmptyTreeHash.length_bytes);
+const std::string& GetEmptyTreeHash() {
+  static const std::string empty_hash = HexToBytes(kSHA256EmptyTreeHash);

[Ryan Sleevi, 2016/08/25 18:32:11]

STYLE:
https://google.github.io/styleguide/cppguide.html#Static_and_Global_Variables

"Variables of class type with static storage duration are forbidden"

[Rob Percival, 2016/08/25 19:35:13]

Fixed by changing kSHA256EmptyTreeHash to be the decoded bytes, and just
returning that. It's less efficient but avoids the static std::string.

[Ryan Sleevi, 2016/08/25 20:15:20]

DESIGN: Given that you're clearly concerned about performance, why not const
char*? :)

[Rob Percival, 2016/08/25 20:26:40]

Passing around hashes as char* runs the risk that it'll get implicitly converted
into a std::string at some point, and there lie subtle bugs when the hash
contains a null byte. The only performance impact I observed was the one caused
by executing HexToBytes for the same empty hash over and over. I'm far less
concerned about the cost of string copies.

+  return empty_hash;
 }
 
 // Creates a ct::MerkleConsistencyProof and returns the result of
 // calling log->VerifyConsistencyProof with that proof and snapshots.
 bool VerifyConsistencyProof(scoped_refptr<const CTLogVerifier> log,
                             uint64_t old_tree_size,
                             const std::string& old_tree_root,
                             uint64_t new_tree_size,
                             const std::string& new_tree_root,
                             const std::vector<std::string>& proof) {
@@ skipping 243 matching lines @@
 }
 
 TEST_F(CTLogVerifierTest, VerifiesValidConsistencyProofs) {
   std::vector<std::string> proof;
   std::string root1, root2;
 
   // Known good proofs.
   for (size_t i = 0; i < arraysize(kSHA256Proofs); ++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));
+      const char* const v = kSHA256Proofs[i].proof[j];
+      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);
+    const char* const old_root = kSHA256Roots[snapshot1 - 1];
+    const char* const new_root = kSHA256Roots[snapshot2 - 1];
+    VerifierConsistencyCheck(snapshot1, snapshot2, HexToBytes(old_root),
+                             HexToBytes(new_root), proof);
   }
 }
 
 const char kLeafPrefix[] = {'\x00'};
 
 // Reference implementation of RFC6962.
 // This allows generation of arbitrary-sized Merkle trees and consistency
 // proofs between them for testing the consistency proof validation
 // code.
 class TreeHasher {
  public:
-  static std::string HashEmpty() {
-    return HexToBytes(kSHA256EmptyTreeHash.str,
-                      kSHA256EmptyTreeHash.length_bytes);
-  }
-
   static std::string HashLeaf(const std::string& leaf) {
     SHA256HashValue sha256;
     memset(sha256.data, 0, sizeof(sha256.data));
 
     std::unique_ptr<crypto::SecureHash> hash(
         crypto::SecureHash::Create(crypto::SecureHash::SHA256));
     hash->Update(kLeafPrefix, 1);
     hash->Update(leaf.data(), leaf.size());
     hash->Finish(sha256.data, sizeof(sha256.data));
 
     return std::string(reinterpret_cast<const char*>(sha256.data),
                        sizeof(sha256.data));
   }
 };
 
 // Reference implementation of Merkle hash, for cross-checking.
 // Recursively calculates the hash of the root given the leaf data
 // specified in |inputs|.
 std::string ReferenceMerkleTreeHash(std::string* inputs, uint64_t input_size) {
   if (!input_size)
-    return TreeHasher::HashEmpty();
+    return GetEmptyTreeHash();
   if (input_size == 1)
     return TreeHasher::HashLeaf(inputs[0]);
 
   const uint64_t split = CalculateNearestPowerOfTwo(input_size);
 
   return ct::internal::HashNodes(
       ReferenceMerkleTreeHash(&inputs[0], split),
       ReferenceMerkleTreeHash(&inputs[split], input_size - split));
 }
 
@@ skipping 72 matching lines @@
 
 // Test verification of consistency proofs between all tree sizes from 1 to 128.
 INSTANTIATE_TEST_CASE_P(RangeOfTreeSizesAndSnapshots,
                         CTLogVerifierTestUsingReferenceGenerator,
                         testing::Range(UINT64_C(1),
                                        (kReferenceTreeSize / 2) + 1));
 
 }  // namespace
 
 }  // namespace net