Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2183433002:
PVer4: RICE decode bytes to list of uint32 or 4-byte hash prefixes (Closed)
|
Created:
4 years, 5 months ago by vakh (use Gerrit instead) Modified:
4 years, 4 months ago CC:
chromium-reviews, woz, Nathan Parker, Scott Hess - ex-Googler, francois_mozilla.com Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPVer4: Implements a RICE Decoder that converts a RICE encoded string, to a list of uint32_t values, or a sequence of 4-byte hash prefixes.
For more details about RICE encoding, see: https://en.wikipedia.org/wiki/Golomb_coding
BUG=543161, 624567
Committed: https://crrev.com/9e71d408737100714f022dffe20dfb346d395bf9
Cr-Commit-Position: refs/heads/master@{#408478}
Patch Set 1 #Patch Set 2 : Add DecodeIntegers and DecodeBytes methods and add unit tests #Patch Set 3 : Tiny: Fix deps #Patch Set 4 : Add comments and add a few more tests #Patch Set 5 : Tiny: Change the type of data_byte_index_ and current_word_bit_index_ to unsigned int #
Total comments: 30
Patch Set 6 : CR feedback from palmer@. Added condition+test for num_entries<0. Reserving space in output to ensu… #Patch Set 7 : Use RepeatedField instead of std::vector #Patch Set 8 : s/size_t/unsigned int: to fix some compile errors on Windows #
Total comments: 13
Patch Set 9 : palmer@ feedback. Use int32 instead of uint32_t/int since that's what the proto offers #Patch Set 10 : Add release mode only tests for cases that hit NOTREACHED #
Total comments: 4
Patch Set 11 : s/RICE/Rice and use safe-math to check integer overflow #
Dependent Patchsets: Messages
Total messages: 75 (48 generated)
Description was changed from ========== PVer4: Rice Decoder from bytes to uint32 BUG=543161 ========== to ========== PVer4: Rice Decoder from bytes to uint32 BUG=543161,624567 ==========
Add DecodeIntegers and DecodeBytes methods and add unit tests
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vakh@chromium.org changed reviewers: + noelutz@chromium.org, palmer@chromium.org
This CL contains the code to decode the RICE hashes. I'll send a follow-up CL that uses the methods in this CL to decode the RICE encoded information sent by the server.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
TODO (vakh): Add comments in the header file to describe each method better. I'll add that information in the next patch.
Tiny: Fix deps
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Add comments and add a few more tests
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
Tiny: Correct the format specifier
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Tiny: Change the type of data_byte_index_ and current_word_bit_index_ to unsigned int
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Patchset #5 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... File components/safe_browsing_db/v4_rice.cc (right): https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:32: return DECODE_OUTPUT_IS_NULL_FAILURE; Does it make sense to DCHECK here? Well-behaved callers would never do this, right? https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:182: // Bits we need after we reading all of current_word_ Grammar: "after we read" Style: Mark identifiers with |...| https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... File components/safe_browsing_db/v4_rice.h (right): https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:7: // This implementation has "M" as a power of 2, where this power is specified as Perhaps you can leave this sentence out, since you describe |rice_parameter| down below where it's used. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:20: // Enumerate different failure events while decoding the rice encoded string Style nit: "Rice-encoded". https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:46: // Decodes the RICE encoded string in |encoded_data| as a list of integers Style nit: Rice-encoded https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:48: // calculating 'M', |num_entries| is the number of encoded entries, and Explaining that it's for calculating M doesn't explain anything additional. To a person who has read the Wikipedia page, which everyone who writes code that calls into this function should do, the name |M| more meaningful than |rice_parameter|. I would just call it |M| or |m|. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:49: // |first_value| is the first value in the sequence (even when |num_entries| What should |first_value| be in the case that |num_entries| is 0? https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:57: // Decodes the RICE encoded string in |encoded_data| as a string of 4-byte Style nit: Rice-encoded https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:85: // Returns true until |num_entries| number of values have been decoded. Style nit: "Returns true until |num_entries_| entries have been decoded. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:92: // Reads in upto 32 bits from |encoded_data| into |word|, from which Style nit: "up to" https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:103: // The rice parameter, which is the exponent of two for calculating 'M'. 'M' Style nit: Rice parameter (but, again, I'd call it |M_| or |m_|. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:111: // The RICE encoded string. Style nit: Rice-encoded https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:114: // Represents how many total bytes have we read from data_ into current_word_. Nit: Mark identifiers with |...|. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:117: // Represents the number of bits that we have read from current_word_. When Same https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:122: // The 32-bit value read from data_. All bit reading operations operate on Same
Also, this CL needs a much more informative description.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
CR feedback from palmer@. Added condition+test for num_entries<0. Reserving space in output to ensure that we don't take more space than necessary
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the review. PTAL. I'm also changing it slightly to use RepeatedPtr<int> instead of vector<int> so that it can work better with the code in v4_store. Not a big change. Patch coming in an hour or so. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... File components/safe_browsing_db/v4_rice.cc (right): https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:32: return DECODE_OUTPUT_IS_NULL_FAILURE; On 2016/07/26 23:59:48, palmer wrote: > Does it make sense to DCHECK here? Well-behaved callers would never do this, > right? Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:182: // Bits we need after we reading all of current_word_ On 2016/07/26 23:59:48, palmer wrote: > Grammar: "after we read" > Style: Mark identifiers with |...| Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... File components/safe_browsing_db/v4_rice.h (right): https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:7: // This implementation has "M" as a power of 2, where this power is specified as On 2016/07/26 23:59:49, palmer wrote: > Perhaps you can leave this sentence out, since you describe |rice_parameter| > down below where it's used. Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:20: // Enumerate different failure events while decoding the rice encoded string On 2016/07/26 23:59:49, palmer wrote: > Style nit: "Rice-encoded". Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:46: // Decodes the RICE encoded string in |encoded_data| as a list of integers On 2016/07/26 23:59:49, palmer wrote: > Style nit: Rice-encoded Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:48: // calculating 'M', |num_entries| is the number of encoded entries, and On 2016/07/26 23:59:49, palmer wrote: > Explaining that it's for calculating M doesn't explain anything additional. To a > person who has read the Wikipedia page, which everyone who writes code that > calls into this function should do, the name |M| more meaningful than > |rice_parameter|. I would just call it |M| or |m|. I see what you mean but calling it |M| would be incorrect since M is 2^rice_parameter, as explained in the comments. I could call it |b|, based on the variable name on the Wikipedia page, but I think that doesn't convey any information to the reader. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:49: // |first_value| is the first value in the sequence (even when |num_entries| On 2016/07/26 23:59:49, palmer wrote: > What should |first_value| be in the case that |num_entries| is 0? Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:57: // Decodes the RICE encoded string in |encoded_data| as a string of 4-byte On 2016/07/26 23:59:48, palmer wrote: > Style nit: Rice-encoded Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:85: // Returns true until |num_entries| number of values have been decoded. On 2016/07/26 23:59:49, palmer wrote: > Style nit: "Returns true until |num_entries_| entries have been decoded. Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:92: // Reads in upto 32 bits from |encoded_data| into |word|, from which On 2016/07/26 23:59:49, palmer wrote: > Style nit: "up to" Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:103: // The rice parameter, which is the exponent of two for calculating 'M'. 'M' On 2016/07/26 23:59:49, palmer wrote: > Style nit: Rice parameter (but, again, I'd call it |M_| or |m_|. See above. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:111: // The RICE encoded string. On 2016/07/26 23:59:49, palmer wrote: > Style nit: Rice-encoded Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:114: // Represents how many total bytes have we read from data_ into current_word_. On 2016/07/26 23:59:49, palmer wrote: > Nit: Mark identifiers with |...|. Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:117: // Represents the number of bits that we have read from current_word_. When On 2016/07/26 23:59:49, palmer wrote: > Same Done. https://codereview.chromium.org/2183433002/diff/100001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:122: // The 32-bit value read from data_. All bit reading operations operate on On 2016/07/26 23:59:48, palmer wrote: > Same Done.
Description was changed from ========== PVer4: Rice Decoder from bytes to uint32 BUG=543161,624567 ========== to ========== PVer4: Implements a RICE Decoder that converts a RICE encoded string, to a list of uint32_t values, or a sequence of 4-byte hash prefixes. For more details about RICE encoding, see: https://en.wikipedia.org/wiki/Golomb_coding BUG=543161,624567 ==========
Use RepeatedField instead of std::vector
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/07/27 00:02:05, palmer wrote: > Also, this CL needs a much more informative description. Done.
s/size_t/unsigned int: to fix some compile errors on Windows
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_rice.cc (right): https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:36: return NUM_ENTRIES_NEGATIVE_FAILURE; Could this be a DCHECK (in addition to a production check as now)? https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:42: if (num_entries > 0) { Nit: You could save a level of indentation by putting something like this: if (num_entries == 0) return DECODE_SUCCESS; But, up to you. https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:50: last_value += offset; Could this arithmetic overflow? (Does it matter if it does?) https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:163: *word = *word & 0xFFFFFFFF; Is this line necessary? (Maybe it is, I'm not sure.) https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:184: // All the bits that we need are in current_word_ Nit: // All the bits that we need are in |current_word_|. https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_rice_unittest.cc (right): https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice_unittest.cc:131: Does it make sense to have tests for DECODE_REQUESTED_TOO_MANY_BITS_FAILURE and/or NUM_ENTRIES_NEGATIVE_FAILURE ?
palmer@ feedback. Use int32 instead of uint32_t/int since that's what the proto offers
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_rice.cc (right): https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:36: return NUM_ENTRIES_NEGATIVE_FAILURE; On 2016/07/27 23:39:40, palmer wrote: > Could this be a DCHECK (in addition to a production check as now)? Done. https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:42: if (num_entries > 0) { On 2016/07/27 23:39:40, palmer wrote: > Nit: You could save a level of indentation by putting something like this: > > if (num_entries == 0) > return DECODE_SUCCESS; > > But, up to you. Done. https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:50: last_value += offset; On 2016/07/27 23:39:40, palmer wrote: > Could this arithmetic overflow? (Does it matter if it does?) Unexpected. Added a DCHECK. https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:163: *word = *word & 0xFFFFFFFF; On 2016/07/27 23:39:40, palmer wrote: > Is this line necessary? (Maybe it is, I'm not sure.) Done. You're right. I had implemented this method slightly differently earlier and it needed that mask at the end. Not anymore. https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:184: // All the bits that we need are in current_word_ On 2016/07/27 23:39:40, palmer wrote: > Nit: > > // All the bits that we need are in |current_word_|. Done. https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_rice_unittest.cc (right): https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice_unittest.cc:131: On 2016/07/27 23:39:40, palmer wrote: > Does it make sense to have tests for DECODE_REQUESTED_TOO_MANY_BITS_FAILURE > and/or NUM_ENTRIES_NEGATIVE_FAILURE ? I'd love to. However, those have a NOTREACHED() before them so in debug mode, any such test would just crash.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Add release mode only tests for cases that hit NOTREACHED
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... File components/safe_browsing_db/v4_rice_unittest.cc (right): https://codereview.chromium.org/2183433002/diff/160001/components/safe_browsi... components/safe_browsing_db/v4_rice_unittest.cc:131: On 2016/07/27 23:39:40, palmer wrote: > Does it make sense to have tests for DECODE_REQUESTED_TOO_MANY_BITS_FAILURE > and/or NUM_ENTRIES_NEGATIVE_FAILURE ? Done! Added release mode only tests.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi... File components/safe_browsing_db/v4_rice.cc (right): https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi... components/safe_browsing_db/v4_rice.cc:121: DCHECK_LT(last_value, last_value + offset); We need to make this a production sanity test, not a DCHECK, in order to really be safe. (And note that your check is only meaningful because integer overflow is defined for unsigned integer types; if these had been signed, this DCHECK would have undefined behavior.) The canonical Chromium way to do integer overflow checking is to use base::CheckedNumeric. I'd advise sticking to that standard way of doing things. https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi... File components/safe_browsing_db/v4_rice.h (right): https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi... components/safe_browsing_db/v4_rice.h:23: // Enumerate different failure events while decoding the RICE-encoded string Nit: Rice-encoded and on line 56, 71, 124, and 132. (Rice is a person's name, not an acronym or initialism)
s/RICE/Rice and use safe-math to check integer overflow
The CQ bit was checked by vakh@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
vakh@chromium.org changed reviewers: + pkasting@chromium.org
pkasting: Please approve the DEPS addition:
'+third_party/protobuf/src/google',
https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi...
File components/safe_browsing_db/v4_rice.cc (right):
https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_rice.cc:121: DCHECK_LT(last_value, last_value +
offset);
On 2016/07/28 19:26:47, palmer wrote:
> We need to make this a production sanity test, not a DCHECK, in order to
really
> be safe. (And note that your check is only meaningful because integer overflow
> is defined for unsigned integer types; if these had been signed, this DCHECK
> would have undefined behavior.)
>
> The canonical Chromium way to do integer overflow checking is to use
> base::CheckedNumeric. I'd advise sticking to that standard way of doing
things.
I didn't know. Thanks for pointing it out.
Also added tests.
https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi...
File components/safe_browsing_db/v4_rice.h (right):
https://codereview.chromium.org/2183433002/diff/200001/components/safe_browsi...
components/safe_browsing_db/v4_rice.h:23: // Enumerate different failure events
while decoding the RICE-encoded string
On 2016/07/28 19:26:47, palmer wrote:
> Nit: Rice-encoded
>
> and on line 56, 71, 124, and 132.
>
> (Rice is a person's name, not an acronym or initialism)
Done.
LGTM for DEPS
LGTM. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by vakh@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== PVer4: Implements a RICE Decoder that converts a RICE encoded string, to a list of uint32_t values, or a sequence of 4-byte hash prefixes. For more details about RICE encoding, see: https://en.wikipedia.org/wiki/Golomb_coding BUG=543161,624567 ========== to ========== PVer4: Implements a RICE Decoder that converts a RICE encoded string, to a list of uint32_t values, or a sequence of 4-byte hash prefixes. For more details about RICE encoding, see: https://en.wikipedia.org/wiki/Golomb_coding BUG=543161,624567 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== PVer4: Implements a RICE Decoder that converts a RICE encoded string, to a list of uint32_t values, or a sequence of 4-byte hash prefixes. For more details about RICE encoding, see: https://en.wikipedia.org/wiki/Golomb_coding BUG=543161,624567 ========== to ========== PVer4: Implements a RICE Decoder that converts a RICE encoded string, to a list of uint32_t values, or a sequence of 4-byte hash prefixes. For more details about RICE encoding, see: https://en.wikipedia.org/wiki/Golomb_coding BUG=543161,624567 Committed: https://crrev.com/9e71d408737100714f022dffe20dfb346d395bf9 Cr-Commit-Position: refs/heads/master@{#408478} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/9e71d408737100714f022dffe20dfb346d395bf9 Cr-Commit-Position: refs/heads/master@{#408478} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
