Apply sizeof to variables instead of types.

net/cert/crl_set_storage.cc

Index: net/cert/crl_set_storage.cc
diff --git a/net/cert/crl_set_storage.cc b/net/cert/crl_set_storage.cc
index 86b24609302e02a0d791e1e18d5053e19e5b029d..99403da3896b7fe6265fbf1ae9e12c2a91402a14 100644
--- a/net/cert/crl_set_storage.cc
+++ b/net/cert/crl_set_storage.cc
@@ -112,11 +112,12 @@ static bool DecompressZlib(uint8_t* out, int out_len, base::StringPiece in) {
 // updates |data| to remove the header on return. Caller takes ownership of the
 // returned pointer.
 static base::DictionaryValue* ReadHeader(base::StringPiece* data) {
-  if (data->size() < 2)
-    return NULL;
   uint16_t header_len;
-  memcpy(&header_len, data->data(), 2);  // Assumes little-endian.
-  data->remove_prefix(2);
+  if (data->size() < sizeof(header_len))
+    return NULL;
+  // Assumes little-endian.
+  memcpy(&header_len, data->data(), sizeof(header_len));
+  data->remove_prefix(sizeof(header_len));
 
   if (data->size() < header_len)
     return NULL;
@@ -145,16 +146,17 @@ static bool ReadCRL(base::StringPiece* data, std::string* out_parent_spki_hash,
   out_parent_spki_hash->assign(data->data(), crypto::kSHA256Length);
   data->remove_prefix(crypto::kSHA256Length);
 
-  if (data->size() < sizeof(uint32_t))
-    return false;
   uint32_t num_serials;
+  if (data->size() < sizeof(num_serials))
+    return false;
   // Assumes little endian.
-  memcpy(&num_serials, data->data(), sizeof(uint32_t));
+  memcpy(&num_serials, data->data(), sizeof(num_serials));
+  data->remove_prefix(sizeof(num_serials));

[eroman, 2015/06/04 22:25:31]

this is a behavioral change from before.
that said, looks fine to me.

[wtc, 2015/06/05 17:39:33]

Confirmed. I think the new code is better (it immediately removes from |data|
the bytes that have been copied out) and is more consistent with the rest of
the parsing code.

+
   if (num_serials > 32 * 1024 * 1024)  // Sanity check.

[eroman, 2015/06/04 22:25:31]

I am curious if you know where this number comes from (i.e. is to bound the
total memory usage)
3.1 million is a lot of serial numbers

[wtc, 2015/06/05 17:39:33]

I don't know where this number comes from. It is a reasonable maximum CRLSet
size and may have been chosen aribitrarily to bound the total memory usage or
download time.

     return false;
 
   out_serials->reserve(num_serials);
-  data->remove_prefix(sizeof(uint32_t));
 
   for (uint32_t i = 0; i < num_serials; ++i) {
     if (data->size() < sizeof(uint8_t))
@@ -214,13 +216,13 @@ static const unsigned kMaxUncompressedChangesLength = 1024 * 1024;
 static bool ReadChanges(base::StringPiece* data,
                         std::vector<uint8_t>* out_changes) {
   uint32_t uncompressed_size, compressed_size;
-  if (data->size() < 2 * sizeof(uint32_t))
+  if (data->size() < sizeof(uncompressed_size) + sizeof(compressed_size))
     return false;
   // Assumes little endian.
-  memcpy(&uncompressed_size, data->data(), sizeof(uint32_t));
-  data->remove_prefix(4);
-  memcpy(&compressed_size, data->data(), sizeof(uint32_t));
-  data->remove_prefix(4);
+  memcpy(&uncompressed_size, data->data(), sizeof(uncompressed_size));
+  data->remove_prefix(sizeof(uncompressed_size));
+  memcpy(&compressed_size, data->data(), sizeof(compressed_size));
+  data->remove_prefix(sizeof(compressed_size));
 
   if (uncompressed_size > kMaxUncompressedChangesLength)
     return false;
@@ -261,10 +263,9 @@ static bool ReadDeltaCRL(base::StringPiece* data,
       out_serials->push_back(old_serials[i]);
       i++;
     } else if (*k == SYMBOL_INSERT) {
-      uint8_t serial_length;
       if (data->size() < sizeof(uint8_t))

[eroman, 2015/06/04 22:25:31]

Consider changing this line too.

[wtc, 2015/06/05 17:39:33]

When reading just one byte, I would just use 1. This is why I don't want to
go too far and make the code less readable.

If we ever change this in the future, please also change lines 162-166.

         return false;
-      memcpy(&serial_length, data->data(), sizeof(uint8_t));
+      uint8_t serial_length = data->data()[0];
       data->remove_prefix(sizeof(uint8_t));

[wtc, 2015/06/04 00:57:29]

See similar code on lines 162-166.

 
       if (data->size() < serial_length)