DCHECK that full hash must be 21 bytes or 32 bytes long.

DCHECK that full hash must be 32 bytes long (URL/Extension SHA256 hash) or 21 bytes long (for IP addresses).

BUG=543161
Review-Url: https://codereview.chromium.org/2614763002
Cr-Commit-Position: refs/heads/master@{#442509}
Committed: https://chromium.googlesource.com/chromium/src/+/d87619bcc75816bd2d7f8afd31d2e04dfe4507bc

[vakh (use Gerrit instead), 2017-01-04 18:50:41]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-04 18:50:59]

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

[vakh (use Gerrit instead), 2017-01-04 18:51:05]

vakh@chromium.org changed reviewers:
+ nparker@chromium.org

[Nathan Parker, 2017-01-04 18:58:46]

lgtm

https://codereview.chromium.org/2614763002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_store.cc (right):

https://codereview.chromium.org/2614763002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_store.cc:767: for (const auto& pair :
hash_prefix_map_) {
Why isn't this causing crashes all the time? Do we have tests that would hit
this?

One idea: Since we're only expecting 32 and 21 byte strings here, we could check
for those.  That'd prevent a new Check*() method from incorrectly passing the
wrong string here, I think.

[commit-bot: I haz the power, 2017-01-04 19:36:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-04 19:36:28]

Dry run: This issue passed the CQ dry run.

[vakh (use Gerrit instead), 2017-01-10 03:45:46]

Change DCHECK to accept 32 or 21 size. Add unit test.

[vakh (use Gerrit instead), 2017-01-10 03:45:48]

The CQ bit was checked by vakh@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-10 03:46:23]

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

[vakh (use Gerrit instead), 2017-01-10 03:47:56]

https://codereview.chromium.org/2614763002/diff/1/components/safe_browsing_db...
File components/safe_browsing_db/v4_store.cc (right):

https://codereview.chromium.org/2614763002/diff/1/components/safe_browsing_db...
components/safe_browsing_db/v4_store.cc:767: for (const auto& pair :
hash_prefix_map_) {
On 2017/01/04 18:58:46, Nathan Parker wrote:
> Why isn't this causing crashes all the time?

Because it is executed only for MatchMalwareIP and that wasn't getting executed
yet.

> Do we have tests that would hit this?

We have tests for MatchMalwareIP but they were replacing the V4Store with a fake
class so they never this this code. Added one in v4_store_unittest.cc now

> 
> One idea: Since we're only expecting 32 and 21 byte strings here, we could
check
> for those.  That'd prevent a new Check*() method from incorrectly passing the
> wrong string here, I think.

Done.

[vakh (use Gerrit instead), 2017-01-10 04:30:43]

The CQ bit was checked by vakh@chromium.org

[vakh (use Gerrit instead), 2017-01-10 04:30:43]

The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org
Link to the patchset: https://codereview.chromium.org/2614763002/#ps20001
(title: "Change DCHECK to accept 32 or 21 size. Add unit test.")

[commit-bot: I haz the power, 2017-01-10 04:31:20]

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

[vakh (use Gerrit instead), 2017-01-10 04:32:10]

Description was changed from

==========
XS: Remove DCHECK that full hash must be 32 bytes long.
It is 21 bytes for IP addresses

BUG=543161
==========

to

==========
DCHECK that full hash must be 32 bytes long (URL/Extension SHA256 hash) or 21
bytes long (for IP addresses).

BUG=543161
==========

[commit-bot: I haz the power, 2017-01-10 06:02:57]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484022643200730,
"parent_rev": "0f4fc75c2a389ec29a9f40531038f9798d402b14", "commit_rev":
"d87619bcc75816bd2d7f8afd31d2e04dfe4507bc"}

[commit-bot: I haz the power, 2017-01-10 06:04:02]

Description was changed from

==========
DCHECK that full hash must be 32 bytes long (URL/Extension SHA256 hash) or 21
bytes long (for IP addresses).

BUG=543161
==========

to

==========
DCHECK that full hash must be 32 bytes long (URL/Extension SHA256 hash) or 21
bytes long (for IP addresses).

BUG=543161

Review-Url: https://codereview.chromium.org/2614763002
Cr-Commit-Position: refs/heads/master@{#442509}
Committed:
https://chromium.googlesource.com/chromium/src/+/d87619bcc75816bd2d7f8afd31d2...
==========

[commit-bot: I haz the power, 2017-01-10 06:04:03]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/d87619bcc75816bd2d7f8afd31d2...