Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1295943002:
Add a function for validating a DER-encoded INTEGER. (Closed)
DescriptionAdd a function for validating a DER-encoded INTEGER.
This also modifies der::ParseUint64() to support the full uint64_t range.
BUG=410574
Committed: https://crrev.com/6a9b7cbc41c4a6fe809d3e7f4765f251c0429371
Cr-Commit-Position: refs/heads/master@{#344081}
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #
Total comments: 4
Patch Set 3 : remove numeric_length as an out-parameter of IsValidInteger #
Total comments: 4
Patch Set 4 : Address final comments (change func name, and out_negative --> negative) #
Messages
Total messages: 19 (6 generated)
eroman@chromium.org changed reviewers: + nharper@chromium.org
@nharper: For the main review. I am using this function in https://codereview.chromium.org/1279963003.
https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.cc#new... net/der/parse_values.cc:158: *out_negative = first_byte & 0x80; I'm not familiar with C++ type conversion rules - does this correctly result in a bool that is 0 or 1, or will it end up being 0 or 0x80? https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:39: NET_EXPORT bool ParseInteger(const Input& in, All of the other Parse* functions here are of the form ParseType(const Input& in, Type* out) (for whatever value of Type). It seems like this is more of a verification or validation function than a parsing function (since it doesn't have an output param that represents the value parsed), and its name should be changed to reflect that. https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values_unitte... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:202: {false, {0}, 0, 0}, I noticed your ParseIntegerTestData initialization doesn't bother to initialize trailing fields that aren't used for that particular test case - should the same thing be done here (i.e. leave off expected_value when should_pass==false)? https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:220: const uint8_t input[16]; Why 16? The maximum length used in those test cases is 2.
https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.cc#new... net/der/parse_values.cc:158: *out_negative = first_byte & 0x80; On 2015/08/14 22:23:41, nharper wrote: > I'm not familiar with C++ type conversion rules - does this correctly result in > a bool that is 0 or 1, or will it end up being 0 or 0x80? Done. I changed to an explicit boolean (since MSCV will probably complain anyway). https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:39: NET_EXPORT bool ParseInteger(const Input& in, On 2015/08/14 22:23:41, nharper wrote: > All of the other Parse* functions here are of the form ParseType(const Input& > in, Type* out) (for whatever value of Type). It seems like this is more of a > verification or validation function than a parsing function (since it doesn't > have an output param that represents the value parsed), and its name should be > changed to reflect that. Done. Renamed to IsValidInteger(). https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values_unitte... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:202: {false, {0}, 0, 0}, On 2015/08/14 22:23:41, nharper wrote: > I noticed your ParseIntegerTestData initialization doesn't bother to initialize > trailing fields that aren't used for that particular test case - should the same > thing be done here (i.e. leave off expected_value when should_pass==false)? Done https://codereview.chromium.org/1295943002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:220: const uint8_t input[16]; On 2015/08/14 22:23:41, nharper wrote: > Why 16? The maximum length used in those test cases is 2. Done (no reason, just a consequence of copy-paste + experimentation)
nharper@chromium.org changed reviewers: + davidben@chromium.org
lgtm David, can you look over IsValidInteger to check for any integer size conversion issues? I seem to recall you catching a few issues like that in my initial integer parsing code.
https://codereview.chromium.org/1295943002/diff/20001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1295943002/diff/20001/net/der/parse_values.h#... net/der/parse_values.h:32: // and false otherwise (zero counts as !negative). Nit: I'd do s/zero counts as !negative/zero is non-negative/ https://codereview.chromium.org/1295943002/diff/20001/net/der/parse_values.h#... net/der/parse_values.h:33: // numeric_length: The minimum number of bytes needed to represent this This seems sort of a strange thing to return. Other than ParseUint64, is there a reason you need it? I'd maybe do that part separately since I think you either treat negative being false as an error or you want in.Length().
https://codereview.chromium.org/1295943002/diff/20001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1295943002/diff/20001/net/der/parse_values.h#... net/der/parse_values.h:32: // and false otherwise (zero counts as !negative). On 2015/08/17 17:20:42, David Benjamin wrote: > Nit: I'd do s/zero counts as !negative/zero is non-negative/ Done. https://codereview.chromium.org/1295943002/diff/20001/net/der/parse_values.h#... net/der/parse_values.h:33: // numeric_length: The minimum number of bytes needed to represent this On 2015/08/17 17:20:42, David Benjamin wrote: > This seems sort of a strange thing to return. Other than ParseUint64, is there a > reason you need it? I'd maybe do that part separately since I think you either > treat negative being false as an error or you want in.Length(). Done: moved it to a separate (internal) function. Please review the differences between patchset 2 and 3 to verify this is what you had in mind.
lgtm https://codereview.chromium.org/1295943002/diff/40001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/1295943002/diff/40001/net/der/parse_values.cc... net/der/parse_values.cc:123: size_t GetNumberOfBytesInInteger(const Input& in) { I would maybe call this GetUnsignedIntegerLength or something like that. Something that explicitly says "unsigned" since you're expressly not trying to allow negative numbers. (A signed 2^160-1's precision in bytes arguably is 21.) https://codereview.chromium.org/1295943002/diff/40001/net/der/parse_values.cc... net/der/parse_values.cc:152: bool IsValidInteger(const Input& in, bool* out_negative) { Nit: negative to match the header, now that you don't have the numeric_length/out_numeric_length problem. (Bleh. I really really wish Chromium used the "out_" convention. It avoids that problem so nicely.)
https://codereview.chromium.org/1295943002/diff/40001/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/1295943002/diff/40001/net/der/parse_values.cc... net/der/parse_values.cc:123: size_t GetNumberOfBytesInInteger(const Input& in) { On 2015/08/18 19:44:15, David Benjamin (slow) wrote: > I would maybe call this GetUnsignedIntegerLength or something like that. > Something that explicitly says "unsigned" since you're expressly not trying to > allow negative numbers. (A signed 2^160-1's precision in bytes arguably is 21.) Done. https://codereview.chromium.org/1295943002/diff/40001/net/der/parse_values.cc... net/der/parse_values.cc:152: bool IsValidInteger(const Input& in, bool* out_negative) { On 2015/08/18 19:44:16, David Benjamin (slow) wrote: > Nit: negative to match the header, now that you don't have the > numeric_length/out_numeric_length problem. > > (Bleh. I really really wish Chromium used the "out_" convention. It avoids that > problem so nicely.) Done.
The CQ bit was checked by eroman@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nharper@chromium.org, davidben@chromium.org Link to the patchset: https://codereview.chromium.org/1295943002/#ps60001 (title: "Address final comments (change func name, and out_negative --> negative)")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295943002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by eroman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1295943002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1295943002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/6a9b7cbc41c4a6fe809d3e7f4765f251c0429371 Cr-Commit-Position: refs/heads/master@{#344081} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
