Closed
Bug 986150
Opened 10 years ago
Closed 10 years ago
mozilla::pkix: test der::OptionalBoolean
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(2 files, 2 obsolete files)
6.03 KB,
patch
|
mmc
:
review+
|
Details | Diff | Splinter Review |
4.99 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
The patch in bug 985021 introduces mozilla::pkix::der::OptionalBoolean to handle cases of BOOLEAN DEFAULT FALSE in encodings (i.e. if the boolean is present and has value false, it is an error). There are more places where this would be useful. We should also cover it in our tests.
Assignee | ||
Comment 1•10 years ago
|
||
Turns out, there's only one place we decode BOOLEAN at all, and it already uses OptionalBoolean, so all that's needed are some tests.
Summary: mozilla::pkix: use and test der::OptionalBoolean in all the places (introduced in bug 985021) → mozilla::pkix: test der::OptionalBoolean
Assignee | ||
Comment 2•10 years ago
|
||
Any other tests related to this you think I should add?
Comment 3•10 years ago
|
||
Comment on attachment 8421955 [details] [diff] [review] test Review of attachment 8421955 [details] [diff] [review]: ----------------------------------------------------------------- For readability, I would restructure so that are - 1 test with the only valid encoding first (with ASSERT_EQ(Success, ...) << "Should accept the only valid encoding for OptionalBoolean" - 1 test with multiple invalid encodings (true boolean and integer), each encoding with allowInvalid = true followed by allowInvalid = false with appropriate ASSERT_EQ output - 1 test with the AtEnd stuff That way it's clear that you're testing allowInvalid orthogonally. Actually I'm not sure if the AtEnd stuff adds much. Seems reasonable to me, though. ::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp @@ +149,5 @@ > + sizeof DER_OPTIONAL_BOOLEAN_PRESENT_TRUE)); > + > + bool value = false; > + ASSERT_EQ(Success, OptionalBoolean(input, false, value)); > + ASSERT_EQ(true, value); Always use ASSERT_TRUE(value) when expecting a boolean. @@ +167,5 @@ > + input.Init(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE, > + sizeof DER_OPTIONAL_BOOLEAN_PRESENT_FALSE)); > + > + bool value = false; > + ASSERT_EQ(Failure, OptionalBoolean(input, false, value)); I would find this test easier to read if combined with the next test. It took my eyes a while to distinguish the input param is being tested. Also, it is nice when doing these to give a string to the assert when it fails. That way it is more self documenting. From http://mxr.mozilla.org/mozilla-central/source/testing/gtest/gtest/include/gtest/gtest.h#1844 ASSERT_EQ(Failure, OptionalBoolean(input, false, value)) << "Should disallow invalid encoding"; ASSERT_EQ(Failure, OptionalBoolean(input, true, value)) << "Should allow invalid encoding"; @@ +193,5 @@ > + > +TEST_F(pkixder_universal_types_tests, OptionalBooleanNotPresentNotAtEnd) > +{ > + const uint8_t DER_INTEGER_05[] = { > + 0x02, // INTEGER I was initially confused why this is INTEGER when the test is for OptionalBoolean. But probably I don't understand OptionalBoolean very well. @@ +202,5 @@ > + Input input; > + ASSERT_EQ(Success, input.Init(DER_INTEGER_05, sizeof DER_INTEGER_05)); > + > + bool value = true; > + ASSERT_EQ(Success, OptionalBoolean(input, true, value)); ASSERT_EQ(Success, OptionalBoolean(input, true, value)) << "Should allow invalid encoding for Integer DER (or something)"; @@ +203,5 @@ > + ASSERT_EQ(Success, input.Init(DER_INTEGER_05, sizeof DER_INTEGER_05)); > + > + bool value = true; > + ASSERT_EQ(Success, OptionalBoolean(input, true, value)); > + ASSERT_EQ(false, value); ASSERT_FALSE(value);
Attachment #8421955 -
Flags: review?(mmc) → review-
Assignee | ||
Comment 4•10 years ago
|
||
Thanks for the review - let me know if this is the kind of thing you were looking for.
Attachment #8421955 -
Attachment is obsolete: true
Attachment #8424993 -
Flags: review?(mmc)
Assignee | ||
Comment 5•10 years ago
|
||
I separated out the patch to fix the comments in the other tests (the DER tags were often mislabeled).
Attachment #8424994 -
Flags: review?(mmc)
Updated•10 years ago
|
Attachment #8424994 -
Flags: review?(mmc) → review+
Comment 6•10 years ago
|
||
Comment on attachment 8424993 [details] [diff] [review] test v2 Review of attachment 8424993 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/test/gtest/pkixder_universal_types_tests.cpp @@ +159,5 @@ > + }; > + > + Input input1; > + ASSERT_EQ(Success, input1.Init(DER_OPTIONAL_BOOLEAN_PRESENT_TRUE, > + sizeof DER_OPTIONAL_BOOLEAN_PRESENT_TRUE)); alignment on sizeof @@ +203,5 @@ > + ASSERT_EQ(Success, input1.Init(DER_OPTIONAL_BOOLEAN_PRESENT_FALSE, > + sizeof DER_OPTIONAL_BOOLEAN_PRESENT_FALSE)); > + bool value; > + // If the second parameter to OptionalBoolean is false, invalid encodings > + // that include the field even when it is the DEFAULT FALSE are rejected. This comment is fine. However, you may find it more readable to omit the comment and do bool allowInvalid = false; ASSERT_EQ(Failure, OptionalBoolean(blah, allowInvalid, value)) allowInvalid = true; ASSERT_EQ(Success, OptionalBoolean(blah, allowInvalid, value)) Either way is OK.
Attachment #8424993 -
Flags: review?(mmc) → review+
Assignee | ||
Comment 7•10 years ago
|
||
Thanks! I added the variable. https://hg.mozilla.org/integration/mozilla-inbound/rev/1214948e42bb https://hg.mozilla.org/integration/mozilla-inbound/rev/2ae2a011788b
Attachment #8424993 -
Attachment is obsolete: true
Attachment #8425009 -
Flags: review+
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/1214948e42bb https://hg.mozilla.org/mozilla-central/rev/2ae2a011788b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in
before you can comment on or make changes to this bug.
Description
•