Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1204573005:
[tracing] use SHA1 instead of base::Hash for memory dump GUIDs (Closed)
|
Created:
5 years, 6 months ago by Primiano Tucci (use gerrit) Modified:
5 years, 6 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, erikwright+watch_chromium.org, ssid Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] use SHA1 instead of base::Hash for memory dump GUIDs
Turns out that base::Hash is very easy to collide.
See crbug.com/503593. Switching to SHA1.
BUG=492102
NOTRY=true
Committed: https://crrev.com/b9f284548f0660f116d9511f464984e17cf59f8c
Cr-Commit-Position: refs/heads/master@{#335676}
Patch Set 1 #
Total comments: 5
Patch Set 2 : zero-copies FTW #Messages
Total messages: 24 (9 generated)
primiano@chromium.org changed reviewers: + petrcermak@chromium.org
One comment and one nit. Thanks, Petr https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:20: for (size_t i = 0; i < sizeof(uint64); ++i) { nit: This relies on the fact that sizeof(uint64) <= kSHA1Length. I think it would be a good idea to add a static assertion like here: https://code.google.com/p/chromium/codesearch#chromium/src/components/variati... https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:22: hash <<= 8; This order "wastes" 1 byte completely, because the last 8 bits will always be zero in the returned value. I think you should reverse the two operations: hash <<= 8; hash |= sha1_bytes[i];
picksi@google.com changed reviewers: + picksi@google.com
https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:23: } Apart from reversing the bytes, isn't this just the same as making a union of the sha1_bytes array and a uint64, and just returning the uint64?
https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:20: for (size_t i = 0; i < sizeof(uint64); ++i) { On 2015/06/23 13:13:43, petrcermak wrote: > nit: This relies on the fact that sizeof(uint64) <= kSHA1Length. I think it > would be a good idea to add a static assertion like here: > https://code.google.com/p/chromium/codesearch#chromium/src/components/variati... Ehh, if either uint64 or SHA1 change their size, England will fall! :P https://codereview.chromium.org/1204573005/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:22: hash <<= 8; Actually picksi@ had a better idea here about using an array.
LGTM. Thanks, Petr
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1204573005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1204573005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Exceeded global retry quota
On 2015/06/23 15:00:02, commit-bot: I haz the power wrote: > Exceeded global retry quota I HATE YOU THESE DAYS CQ SGRUMPF
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1204573005/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
The CQ bit was checked by primiano@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1204573005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b9f284548f0660f116d9511f464984e17cf59f8c Cr-Commit-Position: refs/heads/master@{#335676} |
