Apply sizeof to variables instead of types.

net/cert/crl_set_storage.cc

 // Copyright 2014 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/crl_set_storage.h"
 
 #include "base/base64.h"
 #include "base/format_macros.h"
 #include "base/json/json_reader.h"
 #include "base/numerics/safe_conversions.h"
@@ skipping 94 matching lines @@
 // in turn with a zlib compressed array of options: either the CRL is the same,
 // a new CRL is inserted, the CRL is deleted or the CRL is updated. In the case
 // of an update, the serials in the CRL are considered in the same fashion
 // except there is no delta update of a serial number: they are either
 // inserted, deleted or left the same.
 
 // ReadHeader reads the header (including length prefix) from |data| and
 // 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)
+  uint16_t header_len;
+  if (data->size() < sizeof(header_len))
     return NULL;
-  uint16_t header_len;
-  memcpy(&header_len, data->data(), 2);  // Assumes little-endian.
-  data->remove_prefix(2);
+  // Assumes little-endian.
+  memcpy(&header_len, data->data(), sizeof(header_len));
+  data->remove_prefix(sizeof(header_len));
 
   if (data->size() < header_len)
     return NULL;
 
   const base::StringPiece header_bytes(data->data(), header_len);
   data->remove_prefix(header_len);
 
   scoped_ptr<base::Value> header(base::JSONReader::DeprecatedRead(
       header_bytes, base::JSON_ALLOW_TRAILING_COMMAS));
   if (header.get() == NULL)
     return NULL;
 
   if (!header->IsType(base::Value::TYPE_DICTIONARY))
     return NULL;
   return reinterpret_cast<base::DictionaryValue*>(header.release());
 }
 
 // kCurrentFileVersion is the version of the CRLSet file format that we
 // currently implement.
 static const int kCurrentFileVersion = 0;
 
 static bool ReadCRL(base::StringPiece* data, std::string* out_parent_spki_hash,
                     std::vector<std::string>* out_serials) {
   if (data->size() < crypto::kSHA256Length)
     return false;
   out_parent_spki_hash->assign(data->data(), crypto::kSHA256Length);
   data->remove_prefix(crypto::kSHA256Length);
 
-  if (data->size() < sizeof(uint32_t))
+  uint32_t num_serials;
+  if (data->size() < sizeof(num_serials))
     return false;
-  uint32_t num_serials;
   // 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))
       return false;
 
     uint8_t serial_length = data->data()[0];
     data->remove_prefix(sizeof(uint8_t));
 
     if (data->size() < serial_length)
       return false;
@@ skipping 39 matching lines @@
 }
 
 // kMaxUncompressedChangesLength is the largest changes array that we'll
 // accept. This bounds the number of CRLs in the CRLSet as well as the number
 // of serial numbers in a given CRL.
 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;
   if (data->size() < compressed_size)
     return false;
 
   out_changes->clear();
   if (uncompressed_size == 0)
     return true;
 
@@ skipping 20 matching lines @@
 
   size_t i = 0;
   for (std::vector<uint8_t>::const_iterator k = changes.begin();
        k != changes.end(); ++k) {
     if (*k == SYMBOL_SAME) {
       if (i >= old_serials.size())
         return false;
       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)
         return false;
       const std::string serial(data->data(), serial_length);
       data->remove_prefix(serial_length);
 
       out_serials->push_back(serial);
     } else if (*k == SYMBOL_DELETE) {
       if (i >= old_serials.size())
         return false;
@@ skipping 263 matching lines @@
       memcpy(out + off, j->data(), j->size());
       off += j->size();
     }
   }
 
   CHECK_EQ(off, len);
   return ret;
 }
 
 }  // namespace net