Minor improvements to ct_serialization.cc

net/cert/ct_serialization.cc

Index: net/cert/ct_serialization.cc
diff --git a/net/cert/ct_serialization.cc b/net/cert/ct_serialization.cc
index 235d37e65ad1fc8106a05d602c3c9f04b4e61a48..9da4b73cf8506739a00b872db0cbcdeb950dbbe5 100644
--- a/net/cert/ct_serialization.cc
+++ b/net/cert/ct_serialization.cc
@@ -6,9 +6,11 @@
 
 #include <stdint.h>
 
+#include <algorithm>
 #include <limits>
 
 #include "base/logging.h"
+#include "base/numerics/safe_math.h"
 
 namespace net {
 
@@ -45,6 +47,18 @@ enum SignatureType {
   TREE_HASH = 1,
 };
 
+// Returns the maximum value of an unsigned integer that is |length| bytes long.
+// |length| can be up to 8.
+uint64_t GetUintMaxValue(size_t length) {
+  DCHECK_LE(length, 8u);
+
+  // If length == 8, we can't calculate the max value because that would involve
+  // shifting a uint64_t left by 64 bits, which invokes undefined behaviour (see
+  // https://www.securecoding.cert.org/confluence/x/IRE). Instead, we just hard-
+  // code the result for that case.

[Ryan Sleevi, 2016/01/22 23:32:16]

https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/NH-S6KCkr2M

return length == 8 ? UINT64_MAX : (1u << (length * 8)) - 1;


But really, see below why I think this is unnecessary.

[Rob Percival, 2016/01/25 16:42:29]

Acknowledged.

+  return length == 8 ? 0xffffffffffffffff : (uint64_t(1) << (length * 8)) - 1;
+}
+
 // Reads a TLS-encoded variable length unsigned integer from |in|.
 // The integer is expected to be in big-endian order, which is used by TLS.
 // The bytes read from |in| are discarded (i.e. |in|'s prefix removed)
@@ -54,11 +68,15 @@ template <typename T>
 bool ReadUint(size_t length, base::StringPiece* in, T* out) {
   if (in->size() < length)
     return false;
+  DCHECK_NE(length, 0u);
   DCHECK_LE(length, sizeof(T));
 
-  T result = 0;
-  for (size_t i = 0; i < length; ++i) {
-    result = (result << 8) | static_cast<unsigned char>((*in)[i]);
+  T result = static_cast<uint8_t>((*in)[0]);
+  // This loop only executes if sizeof(T) > 1, because the first operation is
+  // to shift left by 1 byte, which is undefined behaviour if T is a 1 byte
+  // integer.
+  for (size_t i = 1; i < length; ++i) {
+    result = (result << 8) | static_cast<uint8_t>((*in)[i]);
   }
   in->remove_prefix(length);
   *out = result;
@@ -66,14 +84,18 @@ bool ReadUint(size_t length, base::StringPiece* in, T* out) {
 }
 
 // Reads a TLS-encoded field length from |in|.
-// The bytes read from |in| are discarded (i.e. |in|'s prefix removed)
-// |prefix_length| indicates the bytes needed to represent the length (e.g. 3)
+// The bytes read from |in| are discarded (i.e. |in|'s prefix removed).
+// |prefix_length| indicates the bytes needed to represent the length (e.g. 3).
+// Max |prefix_length| is 8.
 // success, returns true and stores the result in |*out|.
 bool ReadLength(size_t prefix_length, base::StringPiece* in, size_t* out) {
-  size_t length;
+  uint64_t length = 0;
   if (!ReadUint(prefix_length, in, &length))
     return false;
-  *out = length;
+  base::CheckedNumeric<size_t> checked_length = length;
+  if (!checked_length.IsValid())
+    return false;
+  *out = checked_length.ValueOrDie();
   return true;
 }
 
@@ -96,7 +118,7 @@ bool ReadFixedBytes(size_t length,
 bool ReadVariableBytes(size_t prefix_length,
                        base::StringPiece* in,
                        base::StringPiece* out) {
-  size_t length;
+  size_t length = 0;
   if (!ReadLength(prefix_length, in, &length))
     return false;
   return ReadFixedBytes(length, in, out);
@@ -176,15 +198,15 @@ bool ConvertSignatureAlgorithm(
 }
 
 // Writes a TLS-encoded variable length unsigned integer to |output|.
-// |length| indicates the size (in bytes) of the integer.
+// |length| indicates the size (in bytes) of the integer. This must be able to
+// accomodate |value|.
 // |value| the value itself to be written.
-template <typename T>
-void WriteUint(size_t length, T value, std::string* output) {
-  DCHECK_LE(length, sizeof(T));
-  DCHECK(length == sizeof(T) || value >> (length * 8) == 0);
+void WriteUint(size_t length, uint64_t value, std::string* output) {
+  // Check that |value| fits into |length| bytes.
+  DCHECK(length >= sizeof(value) || value >> (length * 8) == 0);
 
   for (; length > 0; --length) {
-    output->push_back((value >> ((length - 1)* 8)) & 0xFF);
+    output->push_back((value >> ((length - 1) * 8)) & 0xFF);
   }
 }
 
@@ -195,25 +217,29 @@ void WriteUint(size_t length, T value, std::string* output) {
 // length when reading.
 // If the length of |input| is dynamic and data is expected to follow it,
 // WriteVariableBytes must be used.
-void WriteEncodedBytes(const base::StringPiece& input, std::string* output) {
+// Returns the number of bytes written (the length of |input|).
+size_t WriteEncodedBytes(const base::StringPiece& input, std::string* output) {
   input.AppendToString(output);
+  return input.size();
 }
 
 // Writes a variable-length array to |output|.
-// |prefix_length| indicates the number of bytes needed to represnt the length.
+// |prefix_length| indicates the number of bytes needed to represent the length.
 // |input| is the array itself.
-// If the size of |input| is less than 2^|prefix_length| - 1, encode the
-// length and data and return true. Otherwise, return false.
+// If 1 <= |prefix_length| <= 8 and the size of |input| is less than
+// 2^|prefix_length| - 1, encode the length and data and return true.
+// Otherwise, return false.
 bool WriteVariableBytes(size_t prefix_length,
                         const base::StringPiece& input,
                         std::string* output) {
   size_t input_size = input.size();
-  size_t max_allowed_input_size =
-      static_cast<size_t>(((1 << (prefix_length * 8)) - 1));
-  if (input_size > max_allowed_input_size)
+  size_t max_input_size =
+      GetUintMaxValue(std::min(sizeof(size_t), prefix_length));

[Ryan Sleevi, 2016/01/22 23:32:16]

I don't see why this is necessary. You already know that input_size <=
max_input_size if sizeof(size_t) is less than prefix_length, because that's the
maximum possible value of input_size

That is, line 239 makes no sense if, in the std::min, it chooses sizeof(size_t),
so this is only relevant if prefix_length > sizeof(size_t)

In that case, it's simple:
DCHECK_LE(8, prefix_length);
uint64_t input_size = input.size();
uint64_t max_input_size = (prefix_length == 8) ? UINT64_MAX : ((UINT64_C(1) <<
(prefix length * 8)) - 1);

if (input_size > max_input_size)
  return false;


In the above, the following changes are made:
- Make the type promotions explicit (line 242 is implicitly promoting)
- Use the stdlib constants (UINT64_MAX, UINT64_C)
- Reduce the abstraction

[Rob Percival, 2016/01/25 16:42:29]

Done.

+
+  if (input_size > max_input_size)
     return false;
 
-  WriteUint(prefix_length, input.size(), output);
+  WriteUint(prefix_length, input_size, output);
   WriteEncodedBytes(input, output);
 
   return true;
@@ -291,6 +317,27 @@ bool EncodeLogEntry(const LogEntry& input, std::string* output) {
   return false;
 }
 
+static bool ReadTimeSinceEpoch(base::StringPiece* input, base::Time* output) {

[Ryan Sleevi, 2016/01/22 23:32:16]

Is this abstraction necessary? Will it ever be reused? Seems like it really
belongs in the body where it was.

[Rob Percival, 2016/01/25 16:42:29]

Yes, it is reused in one of my next patches:
https://chromiumcodereview-hr.appspot.com/1576513002/. Signed certificate
timestamps aren't the only CT data structures that contain timestamps.

+  uint64_t time_since_epoch = 0;
+  if (!ReadUint(kTimestampLength, input, &time_since_epoch)) {
+    return false;
+  }

[Ryan Sleevi, 2016/01/22 23:32:16]

no braces

[Rob Percival, 2016/01/25 16:42:29]

Done.

+
+  base::CheckedNumeric<int64_t> time_since_epoch_signed = time_since_epoch;
+
+  if (!time_since_epoch_signed.IsValid()) {
+    DVLOG(1) << "Timestamp value too big to cast to int64_t: "
+             << time_since_epoch;
+    return false;
+  }
+
+  *output =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(time_since_epoch_signed.ValueOrDie());
+
+  return true;
+}
+
 static void WriteTimeSinceEpoch(const base::Time& timestamp,
                                 std::string* output) {
   base::TimeDelta time_since_epoch = timestamp - base::Time::UnixEpoch();
@@ -351,28 +398,17 @@ bool DecodeSignedCertificateTimestamp(
   }
 
   result->version = SignedCertificateTimestamp::SCT_VERSION_1;
-  uint64_t timestamp;
   base::StringPiece log_id;
   base::StringPiece extensions;
   if (!ReadFixedBytes(kLogIdLength, input, &log_id) ||
-      !ReadUint(kTimestampLength, input, &timestamp) ||
-      !ReadVariableBytes(kExtensionsLengthBytes, input,
-                         &extensions) ||
+      !ReadTimeSinceEpoch(input, &result->timestamp) ||
+      !ReadVariableBytes(kExtensionsLengthBytes, input, &extensions) ||
       !DecodeDigitallySigned(input, &result->signature)) {
     return false;
   }
 
-  if (timestamp > static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
-    DVLOG(1) << "Timestamp value too big to cast to int64_t: " << timestamp;
-    return false;
-  }
-
   log_id.CopyToString(&result->log_id);
   extensions.CopyToString(&result->extensions);
-  result->timestamp =
-      base::Time::UnixEpoch() +
-      base::TimeDelta::FromMilliseconds(static_cast<int64_t>(timestamp));
-
   output->swap(result);
   return true;
 }
@@ -381,7 +417,7 @@ bool EncodeSCTListForTesting(const base::StringPiece& sct,
                              std::string* output) {
   std::string encoded_sct;
   return WriteVariableBytes(kSerializedSCTLengthBytes, sct, &encoded_sct) &&
-      WriteVariableBytes(kSCTListLengthBytes, encoded_sct, output);
+         WriteVariableBytes(kSCTListLengthBytes, encoded_sct, output);
 }
 
 }  // namespace ct