Closed Bug 1083539 Opened 6 years ago Closed 6 years ago
Factor out common DER sequence unwrapping logic into reusable functions
See patch. More uses of these functions will appear in the name constraints patches. In particular, NestedExpectTagAndGetValueAtEnd seems pretty specialized, but it is also used in for name constraints.
Attachment #8505823 - Flags: review?(mmc)
Attachment #8505823 - Flags: review?(mmc) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c4a06c58998 Thanks for the quick review!
Comment on attachment 8505823 [details] [diff] [review] factor-out-common-SEQUENCE-unwrapping-logic.patch Review of attachment 8505823 [details] [diff] [review]: ----------------------------------------------------------------- ::: security/pkix/lib/pkixder.h @@ +249,5 @@ > +} > + > +// Similar to the above, but does two levels of unwrapping. > +inline Result > +NestedExpectTagAndGetValueAtEnd(Reader& outer, uint8_t expectedOuterTag, I thought that I needed this NestedExpectTagAndGetValueAtEnd function for the new name constraint implementation. However, it turns out I was wrong. Therefore, I removed it from the patch before checkin. ::: security/pkix/lib/pkixocsp.cpp @@ +411,4 @@ > Reader certsSequence; > + rv = der::NestedExpectTagAndGetValueAtEnd( > + input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0, > + der::SEQUENCE, certsSequence); Because I removed the NestedExpectTagAndGetValueAtEnd function, I replaced this call with two calls to ExpectTagAndGetValueAtEnd.
Assignee: brian → mmc
Status: NEW → ASSIGNED
Attachment #8510701 - Flags: review?(dkeeler)
Attachment #8510701 - Flags: review?(dkeeler) → review+
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #3) > Created attachment 8510701 [details] [diff] [review] > Fix dropped return value check ( Thank you for noticing this, and thanks for fixing it. How did you find it? Is somebody building Gecko in a way that causes this type of error (unused assignment) to be found, in a way that isn't found on tryserver and inbound?
I found it by looking at the diff.
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Yep, we have evilpie to thank.
(In reply to Tom Schuster [:evilpie] from comment #5) > I found it by looking at the diff. An incredible innovation in security! Thanks for finding this! I filed bug 1088845 so that we can make this type of bug never happen again in mozilla::pkix.
You need to log in before you can comment on or make changes to this bug.