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..ebeda1fd234f000ee933f47f21c7595adecbf3e4 100644
--- a/net/cert/ct_serialization.cc
+++ b/net/cert/ct_serialization.cc
@@ -4,8 +4,7 @@
 
 #include "net/cert/ct_serialization.h"
 
-#include <stdint.h>
-
+#include <cstdint>

[Ryan Sleevi, 2016/01/08 19:46:31]

Chromium code recommends stdint.h (and, in general, the C headers over the C++
ones due to the std:: namespacing concerns). They're not entirely isomorphic 

Was there a specific reason for this change?

[Rob Percival, 2016/01/10 03:48:56]

No, I just wasn't aware of that recommendation - I'll revert this change.

 #include <limits>
 
 #include "base/logging.h"
@@ -66,14 +65,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;
+  if (length > std::numeric_limits<size_t>::max()) {
+    return false;
+  }

[Ryan Sleevi, 2016/01/08 19:46:31]

CONSISTENCY: No braces for one-line conditionals

[Rob Percival, 2016/01/10 03:48:56]

Done.

+  *out = static_cast<size_t>(length);
   return true;
 }
 
@@ -96,7 +99,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;

[Ryan Sleevi, 2016/01/08 19:46:31]

Why? (I mean, on a mere pedantry that adds an extra instruction or three for a
case that you also seem to handle above)

[Rob Percival, 2016/01/10 03:48:56]

It's a little more defensive against bugs than trusting that ReadLength will
definitely have assigned to length when it returns true. This avoids a buffer
over-read in ReadFixedBytes. Admittedly, it's almost certainly unnecessary,
since the odds of ReadLength ever changing and introducing a bug like that are
very slim, but there seems no harm in it either and it's a good practice in
general.

   if (!ReadLength(prefix_length, in, &length))
     return false;
   return ReadFixedBytes(length, in, out);
@@ -184,7 +187,7 @@ void WriteUint(size_t length, T value, std::string* output) {
   DCHECK(length == sizeof(T) || 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 +198,33 @@ 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.
 // |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));
+  // Use uint64_t instead of size_t to allow a |prefix_length| up to 8 bytes,
+  // even on 32-bit builds.

[Ryan Sleevi, 2016/01/08 19:46:31]

Why? input.size() will never exceed 4 bytes on 32-bit builds

[Rob Percival, 2016/01/10 03:48:56]

The catch here is that WriteUint (below) is a template function that checks that
sizeof(input_size) >= prefix_length. On a 32-bit build, size_t doesn't exceed 4
bytes, as you say, but prefix_length is sometimes 8, causing a crash.

[Ryan Sleevi, 2016/01/12 00:10:15]

Isn't this moreso a bug in line 186/187 having bad preconditions? The
explanation was a little confusing, but now it's clearer that you're talking
about cases where sizeof(T) == 4, when T = size_t on 32-bit, but prefix_length
== 8. This triggers the DCHECK on line 186.

Put differently, I'm not sure whether it's "correct" to fix this here or whether
to fix WriteUint - my gut is that WriteUint should be robust enough to handle
the size_t mismatch case for all possible inputs.

[Rob Percival, 2016/01/13 00:55:52]

How about we replace the preconditions of WriteUint with either:
a) DCHECK_EQ(value >> (length * 8), 0);
b) DCHECK_GE(length, sizeof(T)); 

I prefer (b), since it's simpler and requires fewer tests, whereas (a) really
requires all calling code to have boundary tests for the values they can pass
in. On the other hand, (b) does require more strictness with the size of
variable you use, e.g. if length == 1, you have to use (u)int8_t or cast to it,
rather than being able to pass in an int or an enum as you can now. Perhaps
that's an improvement though?

+  uint64_t input_size = input.size();
+  if (prefix_length > sizeof(input_size)) {
+    return false;
+  }

[Ryan Sleevi, 2016/01/08 19:46:31]

Consistency: Braces

[Rob Percival, 2016/01/10 03:48:56]

Done.

+
+  uint64_t max_allowed_input_size = ((1 << (prefix_length * 8)) - 1);
   if (input_size > max_allowed_input_size)
     return false;
 
-  WriteUint(prefix_length, input.size(), output);
+  WriteUint(prefix_length, input_size, output);
   WriteEncodedBytes(input, output);
 
   return true;
@@ -291,6 +302,26 @@ bool EncodeLogEntry(const LogEntry& input, std::string* output) {
   return false;
 }
 
+static bool ReadTimeSinceEpoch(base::StringPiece* input, base::Time* output) {
+  uint64_t time_since_epoch = 0;
+  if (!ReadUint(kTimestampLength, input, &time_since_epoch)) {
+    return false;
+  }
+
+  if (time_since_epoch >
+      static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {

[Ryan Sleevi, 2016/01/08 19:46:31]

There's stuff in base/safe_numerics to help with this sort of cast+cast+convert

[Rob Percival, 2016/01/10 03:48:56]

Done.

+    DVLOG(1) << "Timestamp value too big to cast to int64_t: "
+             << time_since_epoch;
+    return false;
+  }
+
+  *output =
+      base::Time::UnixEpoch() +
+      base::TimeDelta::FromMilliseconds(static_cast<int64_t>(time_since_epoch));
+
+  return true;
+}
+
 static void WriteTimeSinceEpoch(const base::Time& timestamp,
                                 std::string* output) {
   base::TimeDelta time_since_epoch = timestamp - base::Time::UnixEpoch();
@@ -351,28 +382,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 +401,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