Minor improvements to ct_serialization.cc

Minor improvements to ct_serialization.cc

- Use uint64_t in place of size_t where we need to guarantee 64-bit
  integers.
- Minor formatting and diagnostic improvements.
- Extracted timestamp decoding into its own function, ReadTimeSinceEpoch.
  This will be used more in future code.

BUG=506227
Committed: https://crrev.com/00a089237741d2b666aab6ee2e20a3a91d816cac
Cr-Commit-Position: refs/heads/master@{#371359}

[Rob Percival, 2016-01-08 19:00:55]

robpercival@chromium.org changed reviewers:
+ rsleevi@chromium.org

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

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.cc
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:7: #include <cstdint>
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?

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:78: }
CONSISTENCY: No braces for one-line conditionals

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:102: size_t length = 0;
Why? (I mean, on a mere pedantry that adds an extra instruction or three for a
case that you also seem to handle above)

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:217: // even on 32-bit builds.
Why? input.size() will never exceed 4 bytes on 32-bit builds

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:221: }
Consistency: Braces

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:312:
static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
There's stuff in base/safe_numerics to help with this sort of cast+cast+convert

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

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.cc
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:7: #include <cstdint>
On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> 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?

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

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:78: }
On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> CONSISTENCY: No braces for one-line conditionals

Done.

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:102: size_t length = 0;
On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> Why? (I mean, on a mere pedantry that adds an extra instruction or three for a
> case that you also seem to handle above)

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.

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:217: // even on 32-bit builds.
On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> Why? input.size() will never exceed 4 bytes on 32-bit builds

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.

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:221: }
On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> Consistency: Braces

Done.

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:312:
static_cast<uint64_t>(std::numeric_limits<int64_t>::max())) {
On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> There's stuff in base/safe_numerics to help with this sort of
cast+cast+convert

Done.

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

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.cc
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1575543002/diff/1/net/cert/ct_serialization.c...
net/cert/ct_serialization.cc:217: // even on 32-bit builds.
On 2016/01/10 03:48:56, Rob Percival wrote:
> On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> > Why? input.size() will never exceed 4 bytes on 32-bit builds
> 
> 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.

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.

https://codereview.chromium.org/1575543002/diff/40001/net/cert/ct_serializati...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1575543002/diff/40001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:210: // |prefix_length| indicates the number of
bytes needed to represnt the length.
typo: represent

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

https://chromiumcodereview-hr.appspot.com/1575543002/diff/1/net/cert/ct_seria...
File net/cert/ct_serialization.cc (right):

https://chromiumcodereview-hr.appspot.com/1575543002/diff/1/net/cert/ct_seria...
net/cert/ct_serialization.cc:217: // even on 32-bit builds.
On 2016/01/12 00:10:15, Ryan Sleevi wrote:
> On 2016/01/10 03:48:56, Rob Percival wrote:
> > On 2016/01/08 19:46:31, Ryan Sleevi wrote:
> > > Why? input.size() will never exceed 4 bytes on 32-bit builds
> > 
> > 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.
> 
> 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.

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?

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
File net/cert/ct_serialization.cc (right):

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
net/cert/ct_serialization.cc:210: // |prefix_length| indicates the number of
bytes needed to represnt the length.
On 2016/01/12 00:10:15, Ryan Sleevi wrote:
> typo: represent

Done.

[Ryan Sleevi, 2016-01-13 02:44:32]

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
File net/cert/ct_serialization.cc (right):

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
net/cert/ct_serialization.cc:193: }
What if we:
Delete both DCHECKs

DCHECK(length >= sizeof(T) || value >> (length * 8) == 0);

size_t bytes_to_write = std::min(length, sizeof(value));
output->insert(output.end(), length - bytes_to_write, 0);
for (; bytes_to_write > 0; --bytes_to_write) {
  output->push_back((value >> ((bytes_to_write - 1) * 8)) & 0xFF);
}

Let's cover possible permutations to sanity check:
length = 3, sizeof(T) == 4:
  - value >> (24) == 0 must be true (DCHECK)
  - bytes_to_write = 3 [length]
  - length - 3 = 0 (so no extra padding)
  - We write 3 bytes

length = 4, sizeof(T) == 4:
  - length == sizeof(T) is true (DCHECK)
  - bytes_to_write = 4 [length or sizeof(T)]
  - length - 4 = 0 (so no extra padding)
  - We write 4 bytes

length = 8, sizeof(T) == 4
  - length > sizeof(T) is true (DCHECK)
  - bytes_to_write = 4 [sizeof(T)]
  - length - 4 = 4 (so 4 bytes of leading zeroes are added to the end)
  - We write 4 additional bytes, the value of T

that should cover the <, =, and > case.

What are your thoughts on that approach? Too much complexity?

I mean, it's unclear to me whether the DCHECK_LE on line 188 is strictly
necessary. That said, I'm not fundamentally opposed to the uint64_t, but I mean,
there's weird stuff (for example, line 221 seems weird - we know
sizeof(input_size) - it's 8)

Alternatively, we could just change the template for WriteUint to not be
variable on type T, explicitly code it to take |value| as a uint64_t (and let
size extension Just Work), and instead just DCHECK that value >> (length * 8) ==
0

Either approach seems more... consistent. But it's late, perhaps I've missed
something.

[Rob Percival, 2016-01-18 11:06:40]

On 2016/01/13 02:44:32, Ryan Sleevi (OOO til 1-19) wrote:
>
https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
> File net/cert/ct_serialization.cc (right):
> 
>
https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
> net/cert/ct_serialization.cc:193: }
> What if we:
> Delete both DCHECKs
> 
> DCHECK(length >= sizeof(T) || value >> (length * 8) == 0);
> 
> size_t bytes_to_write = std::min(length, sizeof(value));
> output->insert(output.end(), length - bytes_to_write, 0);
> for (; bytes_to_write > 0; --bytes_to_write) {
>   output->push_back((value >> ((bytes_to_write - 1) * 8)) & 0xFF);
> }
> 
> Let's cover possible permutations to sanity check:
> length = 3, sizeof(T) == 4:
>   - value >> (24) == 0 must be true (DCHECK)
>   - bytes_to_write = 3 [length]
>   - length - 3 = 0 (so no extra padding)
>   - We write 3 bytes
> 
> length = 4, sizeof(T) == 4:
>   - length == sizeof(T) is true (DCHECK)
>   - bytes_to_write = 4 [length or sizeof(T)]
>   - length - 4 = 0 (so no extra padding)
>   - We write 4 bytes
> 
> length = 8, sizeof(T) == 4
>   - length > sizeof(T) is true (DCHECK)
>   - bytes_to_write = 4 [sizeof(T)]
>   - length - 4 = 4 (so 4 bytes of leading zeroes are added to the end)
>   - We write 4 additional bytes, the value of T
> 
> that should cover the <, =, and > case.
> 
> What are your thoughts on that approach? Too much complexity?
> 
> I mean, it's unclear to me whether the DCHECK_LE on line 188 is strictly
> necessary. That said, I'm not fundamentally opposed to the uint64_t, but I
mean,
> there's weird stuff (for example, line 221 seems weird - we know
> sizeof(input_size) - it's 8)
> 
> Alternatively, we could just change the template for WriteUint to not be
> variable on type T, explicitly code it to take |value| as a uint64_t (and let
> size extension Just Work), and instead just DCHECK that value >> (length * 8)
==
> 0
> 
> Either approach seems more... consistent. But it's late, perhaps I've missed
> something.

The first part of that DCHECK, "length >= sizeof(T)", is redundant because it
implies the second part, "value >> (length * 8) == 0". Therefore, we might as
well make it just "DCHECK(value >> (length * 8) == 0)". Since this only cares
about the value of the integer, and not the type, I'd then be inclined to go
with your suggestion of removing the template parameter from WriteInt and just
taking a uint64_t. This is certainly one of the simplest approaches.

The only drawback I can see is that a particular call to WriteInt might be fine
during unit tests, when it gets called with a |value| within range, but then may
fail the DCHECK in reality if the code allows an out-of-range |value| to get
passed. A safer alternative would be keeping WriteInt templated, and the check
just be "DCHECK_LE(sizeof(value), length)". This ensures that |length| is
sufficient for any |value| that could possibly be passed in.

I'm relatively happy with either of those two approaches, so feel free to pick
one and I'll update this with a new patch set implementing it.

[Ryan Sleevi, 2016-01-19 19:26:32]

On 2016/01/18 11:06:40, Rob Percival wrote:
> The first part of that DCHECK, "length >= sizeof(T)", is redundant because it
> implies the second part, "value >> (length * 8) == 0".

Not quite. (T) could be signed, in which case if length >= sizeof(value) we
could get in the danger zone.

https://www.securecoding.cert.org/confluence/display/c/INT34-C.+Do+not+shift+...

Now, it may be that we don't care to ever write signed ints, at which point,
that argues for the uint64_t approach and not the template.

> The only drawback I can see is that a particular call to WriteInt might be
fine
> during unit tests, when it gets called with a |value| within range, but then
may
> fail the DCHECK in reality if the code allows an out-of-range |value| to get
> passed. A safer alternative would be keeping WriteInt templated, and the check
> just be "DCHECK_LE(sizeof(value), length)". This ensures that |length| is
> sufficient for any |value| that could possibly be passed in.

That expression doesn't need to be a DCHECK, it can be a static_assert, because
value is no longer being considered.

However, I think your proposed change is wrong, because I don't see how we'd
ever write a 24-bit varint - if that matters. It's certainly less flexible, and
is where the uint64_t approach would be better.

So in both arguments you've raised, uint64_t is a better fit than your proposed
solutions, and effectively equivalent to the templated form. In which case, it
may just be easier to have WriteInt take a uint64_t and be done with reasoning
about the edge cases.

[Rob Percival, 2016-01-20 09:47:20]

On 2016/01/19 19:26:32, Ryan Sleevi wrote:
> On 2016/01/18 11:06:40, Rob Percival wrote:
> > The first part of that DCHECK, "length >= sizeof(T)", is redundant because
it
> > implies the second part, "value >> (length * 8) == 0".
> 
> Not quite. (T) could be signed, in which case if length >= sizeof(value) we
> could get in the danger zone.
> 
>
https://www.securecoding.cert.org/confluence/display/c/INT34-C.+Do+not+shift+...

That page says that it's ok to shift a signed value up to its width. It
discourages shifting beyond its precision, because that's *almost* always
indicative of a bug, but isn't in this case. Therefore, no danger zone :)

> Now, it may be that we don't care to ever write signed ints, at which point,
> that argues for the uint64_t approach and not the template.
>
> > The only drawback I can see is that a particular call to WriteInt might be
> fine
> > during unit tests, when it gets called with a |value| within range, but then
> may
> > fail the DCHECK in reality if the code allows an out-of-range |value| to get
> > passed. A safer alternative would be keeping WriteInt templated, and the
check
> > just be "DCHECK_LE(sizeof(value), length)". This ensures that |length| is
> > sufficient for any |value| that could possibly be passed in.
> 
> That expression doesn't need to be a DCHECK, it can be a static_assert,
because
> value is no longer being considered.

Length is being considered though, so it could only be a static_assert if that
became a template parameter. Then WriteVariableBytes would also have to become
templated.

> However, I think your proposed change is wrong, because I don't see how we'd
> ever write a 24-bit varint - if that matters. It's certainly less flexible,
and
> is where the uint64_t approach would be better.
> 
> So in both arguments you've raised, uint64_t is a better fit than your
proposed
> solutions, and effectively equivalent to the templated form. In which case, it
> may just be easier to have WriteInt take a uint64_t and be done with reasoning
> about the edge cases.

The first of my two proposed solutions is exactly that. I'll go ahead and
implement it.

[Rob Percival, 2016-01-20 09:57:05]

On 2016/01/20 09:47:20, Rob Percival wrote:
> On 2016/01/19 19:26:32, Ryan Sleevi wrote:
> > On 2016/01/18 11:06:40, Rob Percival wrote:
> > > The first part of that DCHECK, "length >= sizeof(T)", is redundant because
> it
> > > implies the second part, "value >> (length * 8) == 0".
> > 
> > Not quite. (T) could be signed, in which case if length >= sizeof(value) we
> > could get in the danger zone.
> > 
> >
>
https://www.securecoding.cert.org/confluence/display/c/INT34-C.+Do+not+shift+...
> 
> That page says that it's ok to shift a signed value up to its width. It
> discourages shifting beyond its precision, because that's *almost* always
> indicative of a bug, but isn't in this case. Therefore, no danger zone :)

Shifting signed integers is implementation-defined behavior though, so
admittedly not great. Hardly a pragmatic concern when all of the compilers
supported for Chromium builds have the same behavior but it's one more reason to
go with ditching the template and taking a uint64_t parameter instead. 

> > Now, it may be that we don't care to ever write signed ints, at which point,
> > that argues for the uint64_t approach and not the template.
> >
> > > The only drawback I can see is that a particular call to WriteInt might be
> > fine
> > > during unit tests, when it gets called with a |value| within range, but
then
> > may
> > > fail the DCHECK in reality if the code allows an out-of-range |value| to
get
> > > passed. A safer alternative would be keeping WriteInt templated, and the
> check
> > > just be "DCHECK_LE(sizeof(value), length)". This ensures that |length| is
> > > sufficient for any |value| that could possibly be passed in.
> > 
> > That expression doesn't need to be a DCHECK, it can be a static_assert,
> because
> > value is no longer being considered.
> 
> Length is being considered though, so it could only be a static_assert if that
> became a template parameter. Then WriteVariableBytes would also have to become
> templated.
> 
> > However, I think your proposed change is wrong, because I don't see how we'd
> > ever write a 24-bit varint - if that matters. It's certainly less flexible,
> and
> > is where the uint64_t approach would be better.
> > 
> > So in both arguments you've raised, uint64_t is a better fit than your
> proposed
> > solutions, and effectively equivalent to the templated form. In which case,
it
> > may just be easier to have WriteInt take a uint64_t and be done with
reasoning
> > about the edge cases.
> 
> The first of my two proposed solutions is exactly that. I'll go ahead and
> implement it.

[Ryan Sleevi, 2016-01-20 16:57:39]

On 2016/01/20 09:57:05, Rob Percival wrote:
> On 2016/01/20 09:47:20, Rob Percival wrote:
> > On 2016/01/19 19:26:32, Ryan Sleevi wrote:
> > > On 2016/01/18 11:06:40, Rob Percival wrote:
> > > > The first part of that DCHECK, "length >= sizeof(T)", is redundant
because
> > it
> > > > implies the second part, "value >> (length * 8) == 0".
> > > 
> > > Not quite. (T) could be signed, in which case if length >= sizeof(value)
we
> > > could get in the danger zone.
> > > 
> > >
> >
>
https://www.securecoding.cert.org/confluence/display/c/INT34-C.+Do+not+shift+...
> > 
> > That page says that it's ok to shift a signed value up to its width. It
> > discourages shifting beyond its precision, because that's *almost* always
> > indicative of a bug, but isn't in this case. Therefore, no danger zone :)

Sorry I did not make it clearer.

If (T) is signed, and length > sizeof(T), but you removed that first part of the
DCHECK (as you suggested it was redundant), then value >> (length * 8) would be
shifting value greater than its width - danger zone.

The first clause of the DCHECK serves as a short-circuit for that, avoiding the
undefined shift.

Just wanted to make it clearer why your proposal (admittedly, not the one you
were implementing) would have been wrong, as the first part of that DCHECK did
have effect and was not redundant, as you suggested :)

[Rob Percival, 2016-01-20 20:20:33]

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
File net/cert/ct_serialization.cc (left):

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
net/cert/ct_serialization.cc:214: return false;
It's not clear to me why WriteVariableBytes returns false when prefix_length is
too small, but WriteUint DCHECKs. Any thoughts? Should I make them both either
DCHECK or return a bool?

[Ryan Sleevi, 2016-01-20 20:56:37]

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
File net/cert/ct_serialization.cc (left):

https://chromiumcodereview-hr.appspot.com/1575543002/diff/40001/net/cert/ct_s...
net/cert/ct_serialization.cc:214: return false;
On 2016/01/20 20:20:33, Rob Percival wrote:
> It's not clear to me why WriteVariableBytes returns false when prefix_length
is
> too small, but WriteUint DCHECKs. Any thoughts? Should I make them both either
> DCHECK or return a bool?

I believe the logic was because WriteVariableBytes was presumed to be affected
more by 'user input' than WriteUint.

Even with the DCHECK, for unsigned types, WriteUint still *did the right thing*
(since value of a unsigned integer shifted than greater its size just
zero-extended and thus added zero padding)

I don't know if the perceived inconsistency is worth fixing, in that it results
in correct behaviour but guards against obvious badness. If you strongly feel it
is, that's really for a separate CL.

[Rob Percival, 2016-01-21 09:52:50]

PTAL

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

https://codereview.chromium.org/1575543002/diff/60001/net/cert/ct_serializati...
File net/cert/ct_serialization.cc (right):

https://codereview.chromium.org/1575543002/diff/60001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:58: // code the result for that case.
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.

https://codereview.chromium.org/1575543002/diff/60001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:237: GetUintMaxValue(std::min(sizeof(size_t),
prefix_length));
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

https://codereview.chromium.org/1575543002/diff/60001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:320: static bool
ReadTimeSinceEpoch(base::StringPiece* input, base::Time* output) {
Is this abstraction necessary? Will it ever be reused? Seems like it really
belongs in the body where it was.

https://codereview.chromium.org/1575543002/diff/60001/net/cert/ct_serializati...
net/cert/ct_serialization.cc:324: }
no braces

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

https://chromiumcodereview-hr.appspot.com/1575543002/diff/60001/net/cert/ct_s...
File net/cert/ct_serialization.cc (right):

https://chromiumcodereview-hr.appspot.com/1575543002/diff/60001/net/cert/ct_s...
net/cert/ct_serialization.cc:58: // code the result for that case.
On 2016/01/22 23:32:16, Ryan Sleevi wrote:
>
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.

Acknowledged.

https://chromiumcodereview-hr.appspot.com/1575543002/diff/60001/net/cert/ct_s...
net/cert/ct_serialization.cc:237: GetUintMaxValue(std::min(sizeof(size_t),
prefix_length));
On 2016/01/22 23:32:16, Ryan Sleevi wrote:
> 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

Done.

https://chromiumcodereview-hr.appspot.com/1575543002/diff/60001/net/cert/ct_s...
net/cert/ct_serialization.cc:320: static bool
ReadTimeSinceEpoch(base::StringPiece* input, base::Time* output) {
On 2016/01/22 23:32:16, Ryan Sleevi wrote:
> Is this abstraction necessary? Will it ever be reused? Seems like it really
> belongs in the body where it was.

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.

https://chromiumcodereview-hr.appspot.com/1575543002/diff/60001/net/cert/ct_s...
net/cert/ct_serialization.cc:324: }
On 2016/01/22 23:32:16, Ryan Sleevi wrote:
> no braces

Done.

[Ryan Sleevi, 2016-01-25 21:25:58]

LGTM. Sorry for the LONG back and forth of a 'simple' cleanup ;)

[Rob Percival, 2016-01-25 22:13:34]

The CQ bit was checked by robpercival@chromium.org

[commit-bot: I haz the power, 2016-01-25 22:14:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1575543002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1575543002/80001

[commit-bot: I haz the power, 2016-01-26 00:09:50]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-26 00:10:46]

Description was changed from

==========
Minor improvements to ct_serialization.cc

- Use uint64_t in place of size_t where we need to guarantee 64-bit
  integers.
- Minor formatting and diagnostic improvements.
- Extracted timestamp decoding into its own function, ReadTimeSinceEpoch.
  This will be used more in future code.

BUG=506227
==========

to

==========
Minor improvements to ct_serialization.cc

- Use uint64_t in place of size_t where we need to guarantee 64-bit
  integers.
- Minor formatting and diagnostic improvements.
- Extracted timestamp decoding into its own function, ReadTimeSinceEpoch.
  This will be used more in future code.

BUG=506227

Committed: https://crrev.com/00a089237741d2b666aab6ee2e20a3a91d816cac
Cr-Commit-Position: refs/heads/master@{#371359}
==========

[commit-bot: I haz the power, 2016-01-26 00:10:47]

Patchset 5 (id:??) landed as
https://crrev.com/00a089237741d2b666aab6ee2e20a3a91d816cac
Cr-Commit-Position: refs/heads/master@{#371359}