Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
5 years, 5 months ago by eroman Modified:
5 years, 4 months ago Reviewers:
nharper, davidben, Ryan Sleevi CC:
chromium-reviews, cbentzel+watch_chromium.org, Ryan Sleevi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd functions for DER parsing a BIT STRING.
BUG=410574
Committed: https://crrev.com/5915fe2e06f1df371507836c28873707c77787c1
Cr-Commit-Position: refs/heads/master@{#340594}
Patch Set 1 #
Total comments: 16
Patch Set 2 : Address nharper's comments #
Total comments: 8
Patch Set 3 : Address David's comment #
Created: 5 years, 4 months ago
Messages
Total messages: 17 (6 generated)
eroman@chromium.org changed reviewers: + nharper@chromium.org, rsleevi@chromium.org
@nharper for the primary review @rsleevi for eventual approval (as Nick is not a full committer yet). Feel free to disregard this review until Nick signs off.
https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new... net/der/parse_values.cc:173: // It must be in the range 0 - 7 (or else the encoding is not minimal). cite spec (this is a requirement for BER, not just DER) https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new... net/der/parse_values.cc:184: // Ensure that unused bits in the last byte are set to 0. cite spec (this is only a requirement for CER and DER) https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:44: uint8_t* unused_bits) WARN_UNUSED_RESULT; Should unused_bits be a size_t* instead? I realize that it's on the wire as a uint8_t, but a size_t makes more sense semantically. (This would apply to the methods in parser.h as well.) https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:199: TEST(ParseValuesTest, ParseBitString_Empty_NoUnusedBits) { Test names shouldn't contain underscores (according to https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_..., although I noticed that there are several tests in //net that has underscores in their names.) https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:216: ASSERT_FALSE(ParseBitString(Input(kData), &bytes, &unused_bits)); I generally prefer the EXPECT_* macros to ASSERT_* where feasible to allow the test to continue running. I guess it doesn't really matter when it's the last line of a test case though. https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc File net/der/parser.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc#newcode200 net/der/parser.cc:200: return unused_bits == 0; This approach results in modifying *bytes even if the method returns false. I believe that the rest of the Parser methods only modify output parameters if they return true. (ParseBitString does this correctly by declaring Input bytes (parse_values.cc:180) and only assigning it to the output parameter if the function is going to return true (parse_values.cc:195).) https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc File net/der/parser_unittest.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#... net/der/parser_unittest.cc:206: ASSERT_FALSE(parser.HasMore()); EXPECT_FALSE https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#... net/der/parser_unittest.cc:211: ASSERT_EQ(0xBE, bytes.UnsafeData()[1]); EXPECT_EQ (for lines 209-211) Only parser.ReadBitString and bytes.Length need to ASSERT.
https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc File net/der/parse_values.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new... net/der/parse_values.cc:173: // It must be in the range 0 - 7 (or else the encoding is not minimal). On 2015/07/21 23:23:47, nharper wrote: > cite spec (this is a requirement for BER, not just DER) Done. I changed the comment to: // From ITU-T X.690, section 8.6.2.2 (applies to BER, CER, DER): // // The initial octet shall encode, as an unsigned binary integer with // bit 1 as the least significant bit, the number of unused bits in the final // subsequent octet. The number shall be in the range zero to seven. https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.cc#new... net/der/parse_values.cc:184: // Ensure that unused bits in the last byte are set to 0. On 2015/07/21 23:23:47, nharper wrote: > cite spec (this is only a requirement for CER and DER) Done. I added two citations for this section: // From ITU-T X.690, section 11.2.1 (applies to CER and DER, but not BER): // // Each unused bit in the final octet of the encoding of a bit string value // shall be set to zero. and // From ITU-T X.690, section 8.6.2.3 (applies to BER, CER, DER): // // If the bitstring is empty, there shall be no subsequent octets, // and the initial octet shall be zero. https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values.h#newc... net/der/parse_values.h:44: uint8_t* unused_bits) WARN_UNUSED_RESULT; On 2015/07/21 23:23:47, nharper wrote: > Should unused_bits be a size_t* instead? I realize that it's on the wire as a > uint8_t, but a size_t makes more sense semantically. (This would apply to the > methods in parser.h as well.) I would argue for a uint8_t on the grounds that it is sufficient to represent the needed range. A size_t in my mind implies a length/offset in memory, which is not the case here. It may also be overkill to use a (potentially) 64-bit number to represent a total of 8 total values :) https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte... File net/der/parse_values_unittest.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:199: TEST(ParseValuesTest, ParseBitString_Empty_NoUnusedBits) { On 2015/07/21 23:23:47, nharper wrote: > Test names shouldn't contain underscores (according to > https://code.google.com/p/googletest/wiki/FAQ#Why_should_not_test_case_names_..., > although I noticed that there are several tests in //net that has underscores in > their names.) Ah, thanks for pointing this out! (I was not aware of that) Removed all the underscores. https://codereview.chromium.org/1248043002/diff/1/net/der/parse_values_unitte... net/der/parse_values_unittest.cc:216: ASSERT_FALSE(ParseBitString(Input(kData), &bytes, &unused_bits)); On 2015/07/21 23:23:47, nharper wrote: > I generally prefer the EXPECT_* macros to ASSERT_* where feasible to allow the > test to continue running. I guess it doesn't really matter when it's the last > line of a test case though. Done. These are the result of copy-paste from success test cases. (For which if ParseBitString were to fail, I don't want to proceed as the unused bit length could be invalid memory). https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc File net/der/parser.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parser.cc#newcode200 net/der/parser.cc:200: return unused_bits == 0; On 2015/07/21 23:23:47, nharper wrote: > This approach results in modifying *bytes even if the method returns false. I > believe that the rest of the Parser methods only modify output parameters if > they return true. (ParseBitString does this correctly by declaring Input bytes > (parse_values.cc:180) and only assigning it to the output parameter if the > function is going to return true (parse_values.cc:195).) Done. https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc File net/der/parser_unittest.cc (right): https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#... net/der/parser_unittest.cc:206: ASSERT_FALSE(parser.HasMore()); On 2015/07/21 23:23:47, nharper wrote: > EXPECT_FALSE Done. https://codereview.chromium.org/1248043002/diff/1/net/der/parser_unittest.cc#... net/der/parser_unittest.cc:211: ASSERT_EQ(0xBE, bytes.UnsafeData()[1]); On 2015/07/21 23:23:47, nharper wrote: > EXPECT_EQ (for lines 209-211) > Only parser.ReadBitString and bytes.Length need to ASSERT. Done.
I'll defer to sleevi for size_t/uint8_t in ParseBitString; otherwise LGTM.
eroman@chromium.org changed reviewers: + davidben@chromium.org - rsleevi@chromium.org
Tentatively changing reviewer from rsleevi --> davidben. Because I know David is bored right now and needs more code reviews to keep him busy.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
lgtm
davidben@chromium.org changed reviewers: - rsleevi@chromium.org
lgtm https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h#... net/der/parse_values.h:34: // octet string into |bytes|, and the number of unused bits into |unused_bits|. Probably should do: octet string -> octet string (with bits within each octet ordered from most to least significant, as in the DER encoding) https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h File net/der/parser.h (right): https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h#newcod... net/der/parser.h:155: // bits (in the range 0-7) that are unused. Probably add: The bits are ordered within each octet of |bytes| from most to least significant, as in the DER encoding. https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest.cc File net/der/parser_unittest.cc (right): https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest... net/der/parser_unittest.cc:201: Parser parser((Input(der))); [I'm assuming the extra parens here are coming from the Most Vexing Parse.] https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest... net/der/parser_unittest.cc:229: EXPECT_FALSE(parser.ReadBitStringNoUnusedBits(&bytes)); I dunno if you want a success test for NoUnusedBits.
https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h File net/der/parse_values.h (right): https://codereview.chromium.org/1248043002/diff/20001/net/der/parse_values.h#... net/der/parse_values.h:34: // octet string into |bytes|, and the number of unused bits into |unused_bits|. On 2015/07/27 20:19:59, David Benjamin wrote: > Probably should do: octet string -> octet string (with bits within each octet > ordered from most to least significant, as in the DER encoding) Done (although used the other version of your comment, so it matches parser.h). https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h File net/der/parser.h (right): https://codereview.chromium.org/1248043002/diff/20001/net/der/parser.h#newcod... net/der/parser.h:155: // bits (in the range 0-7) that are unused. On 2015/07/27 20:19:59, David Benjamin wrote: > Probably add: The bits are ordered within each octet of |bytes| from most to > least significant, as in the DER encoding. Done. https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest.cc File net/der/parser_unittest.cc (right): https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest... net/der/parser_unittest.cc:201: Parser parser((Input(der))); On 2015/07/27 20:19:59, David Benjamin wrote: > [I'm assuming the extra parens here are coming from the Most Vexing Parse.] Correct. I need those parenthesis to avoid a compile problem :( /me shakes fist at C++ and its function ptr declarations... https://codereview.chromium.org/1248043002/diff/20001/net/der/parser_unittest... net/der/parser_unittest.cc:229: EXPECT_FALSE(parser.ReadBitStringNoUnusedBits(&bytes)); On 2015/07/27 20:19:59, David Benjamin wrote: > I dunno if you want a success test for NoUnusedBits. Done. I also improved the test descriptions.
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, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1248043002/#ps40001 (title: "Address David's comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1248043002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1248043002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/5915fe2e06f1df371507836c28873707c77787c1 Cr-Commit-Position: refs/heads/master@{#340594} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
