Improve documentation and readability of CTLogVerifier tests

Improve documentation and readability of CTLogVerifier tests

BUG=631087
Committed: https://crrev.com/716ab65db70d4786e04cdf8cf343567b91c49bc7
Cr-Commit-Position: refs/heads/master@{#414741}

[Rob Percival, 2016-07-26 15:33:34]

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

[Rob Percival, 2016-07-26 15:33:35]

PTAL - minor documentation and code changes to improve readability. Extracted
from https://codereview.chromium.org/2182533002/.

[Eran Messeri, 2016-07-26 15:53:17]

lgtm

[Ryan Sleevi, 2016-07-26 18:25:14]

I'm trying to refrain from commenting too strongly, in the event I've misread
something. Please see below, though, to see if I've made a fundamental error.

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
File net/cert/ct_log_verifier_unittest.cc (left):

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
net/cert/ct_log_verifier_unittest.cc:120: kSHA256EmptyTreeHash.length_bytes);
Wait, so this whole time, the empty tree hash was truncated by 32-bytes, and
nothing failed?

[Eran Messeri, 2016-07-26 18:32:08]

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
File net/cert/ct_log_verifier_unittest.cc (left):

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
net/cert/ct_log_verifier_unittest.cc:120: kSHA256EmptyTreeHash.length_bytes);
On 2016/07/26 18:25:14, Ryan Sleevi (extremely slow) wrote:
> Wait, so this whole time, the empty tree hash was truncated by 32-bytes, and
> nothing failed?

Turns out it wasn't, because of a bug in the HexToBytes variant that takes a
(const char*, size_t):
It constructed  hex_data_input of type std::string using only half the hex bytes
(truncating it), but never used the hex_data_input string, passing the input
const char* instead to base::HexStringToBytes (which presumably constructed a
std::string with the entire hex string as it is null-terminated).

[Ryan Sleevi, 2016-07-26 18:36:24]

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
File net/cert/ct_log_verifier_unittest.cc (left):

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
net/cert/ct_log_verifier_unittest.cc:120: kSHA256EmptyTreeHash.length_bytes);
On 2016/07/26 18:32:08, Eran Messeri wrote:
> On 2016/07/26 18:25:14, Ryan Sleevi (extremely slow) wrote:
> > Wait, so this whole time, the empty tree hash was truncated by 32-bytes, and
> > nothing failed?
> 
> Turns out it wasn't, because of a bug in the HexToBytes variant that takes a
> (const char*, size_t):
> It constructed  hex_data_input of type std::string using only half the hex
bytes
> (truncating it), but never used the hex_data_input string, passing the input
> const char* instead to base::HexStringToBytes (which presumably constructed a
> std::string with the entire hex string as it is null-terminated).

So why does this CL leave the bug in place then? (Line 112)

My gripe is that from a reviewers point, there's something that very glaringly
seems a behaviour change (the *2), without documenting in the CL description or
reviewer notes. Even if it's benign, that's totally something that *should* have
been documented, to avoid the first set of questions. Arguably, however, if
we're saying it's a bug, we shouldn't add a workaround for the bug (line 118)
and not even fix the bug (line 112) - pick one or the other. Either we should
remove the length variant from HexToBytes or properly use hex_data_input

[Eran Messeri, 2016-07-26 18:40:02]

> 
> So why does this CL leave the bug in place then? (Line 112)
> 
> My gripe is that from a reviewers point, there's something that very glaringly
> seems a behaviour change (the *2), without documenting in the CL description
or
> reviewer notes. Even if it's benign, that's totally something that *should*
have
> been documented, to avoid the first set of questions. Arguably, however, if
> we're saying it's a bug, we shouldn't add a workaround for the bug (line 118)
> and not even fix the bug (line 112) - pick one or the other. Either we should
> remove the length variant from HexToBytes or properly use hex_data_input

I agree, no reason to leave this bug in place.

[Ryan Sleevi, 2016-07-26 18:40:29]

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
File net/cert/ct_log_verifier_unittest.cc (right):

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
net/cert/ct_log_verifier_unittest.cc:498:
MAYBE_VerifiesValidConsistencyProofsFromReferenceGenerator) {
Since you're improving documentation, this test is woefully under-documented as
to what's being tested.

It came up on the caching CL that it's not entirely obvious what's being tested
(it looks like you're testing the path from every possible leaf to the root, but
it's entirely probable I'm misreading this.

[Ryan Sleevi, 2016-08-01 18:57:08]

Poke? Once updated let me know :)

[Rob Percival, 2016-08-25 16:23:34]

PTAL - there's not much to this any more.

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
File net/cert/ct_log_verifier_unittest.cc (left):

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
net/cert/ct_log_verifier_unittest.cc:120: kSHA256EmptyTreeHash.length_bytes);
On 2016/07/26 18:36:24, Ryan Sleevi (slow) wrote:
> On 2016/07/26 18:32:08, Eran Messeri wrote:
> > On 2016/07/26 18:25:14, Ryan Sleevi (extremely slow) wrote:
> > > Wait, so this whole time, the empty tree hash was truncated by 32-bytes,
and
> > > nothing failed?
> > 
> > Turns out it wasn't, because of a bug in the HexToBytes variant that takes a
> > (const char*, size_t):
> > It constructed  hex_data_input of type std::string using only half the hex
> bytes
> > (truncating it), but never used the hex_data_input string, passing the input
> > const char* instead to base::HexStringToBytes (which presumably constructed
a
> > std::string with the entire hex string as it is null-terminated).
> 
> So why does this CL leave the bug in place then? (Line 112)
> 
> My gripe is that from a reviewers point, there's something that very glaringly
> seems a behaviour change (the *2), without documenting in the CL description
or
> reviewer notes. Even if it's benign, that's totally something that *should*
have
> been documented, to avoid the first set of questions. Arguably, however, if
> we're saying it's a bug, we shouldn't add a workaround for the bug (line 118)
> and not even fix the bug (line 112) - pick one or the other. Either we should
> remove the length variant from HexToBytes or properly use hex_data_input

I've broken out a fix for this into https://codereview.chromium.org/2275353002.
I'll rebase this change on top of that, so that this is pretty much just a
documentation change.

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
File net/cert/ct_log_verifier_unittest.cc (right):

https://codereview.chromium.org/2183073002/diff/1/net/cert/ct_log_verifier_un...
net/cert/ct_log_verifier_unittest.cc:498:
MAYBE_VerifiesValidConsistencyProofsFromReferenceGenerator) {
On 2016/07/26 18:40:29, Ryan Sleevi (slow) wrote:
> Since you're improving documentation, this test is woefully under-documented
as
> to what's being tested.
> 
> It came up on the caching CL that it's not entirely obvious what's being
tested
> (it looks like you're testing the path from every possible leaf to the root,
but
> it's entirely probable I'm misreading this.

Done.

[Ryan Sleevi, 2016-08-25 18:36:14]

lgtm

[Rob Percival, 2016-08-26 16:39:54]

The CQ bit was checked by robpercival@chromium.org

[Rob Percival, 2016-08-26 16:39:55]

The patchset sent to the CQ was uploaded after l-g-t-m from eranm@chromium.org,
rsleevi@chromium.org
Link to the patchset:
https://chromiumcodereview-hr.appspot.com/2183073002/#ps60001 (title: "Rebase")

[commit-bot: I haz the power, 2016-08-26 16:40:43]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-26 17:33:43]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-08-26 17:35:47]

Description was changed from

==========
Improve documentation and readability of CTLogVerifier tests

BUG=631087
==========

to

==========
Improve documentation and readability of CTLogVerifier tests

BUG=631087

Committed: https://crrev.com/716ab65db70d4786e04cdf8cf343567b91c49bc7
Cr-Commit-Position: refs/heads/master@{#414741}
==========

[commit-bot: I haz the power, 2016-08-26 17:35:48]

Patchset 4 (id:??) landed as
https://crrev.com/716ab65db70d4786e04cdf8cf343567b91c49bc7
Cr-Commit-Position: refs/heads/master@{#414741}