Closed
Bug 1154399
Opened 7 years ago
Closed 7 years ago
Clean up pkixder.h
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: briansmith, Assigned: briansmith)
Details
Attachments
(4 files)
3.10 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
3.83 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
4.08 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
2.39 KB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
1. Remove unnecessary templates. 2. Replace old code patterns with new ones. 3. Move code that doesn't need to be inline to pkixder.cpp where practical.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8592368 -
Flags: review?(dkeeler)
Assignee | ||
Comment 2•7 years ago
|
||
Attachment #8592369 -
Flags: review?(dkeeler)
Assignee | ||
Comment 3•7 years ago
|
||
Attachment #8592370 -
Flags: review?(dkeeler)
Assignee | ||
Comment 4•7 years ago
|
||
Attachment #8592372 -
Flags: review?(dkeeler)
Assignee | ||
Comment 5•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f783e1d81dd I've noticed that OS X builds are often timing out on tryserver lately, so I'm just ignoring those.
Attachment #8592368 -
Flags: review?(dkeeler) → review+
Attachment #8592369 -
Flags: review?(dkeeler) → review+
Comment on attachment 8592370 [details] [diff] [review] Part 3: Simplify OptionalExtensions Review of attachment 8592370 [details] [diff] [review]: ----------------------------------------------------------------- This looks great.
Attachment #8592370 -
Flags: review?(dkeeler) → review+
Comment on attachment 8592372 [details] [diff] [review] Part 4: Simplify certificate parsing in OCSP responses Review of attachment 8592372 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. ::: security/pkix/lib/pkixocsp.cpp @@ +410,3 @@ > NonOwningDERArray certs; > if (!input.AtEnd()) { > + rv = der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0, If I'm reading this correctly, we now don't check that we've consumed all of input (whereas I believe ExpectTagAndGetValueAtEnd did). (Ah, but I see that BasicResponse is called from within a call to Nested, which does check that we consume all of this input by the time we return, so I think this is ok.)
Attachment #8592372 -
Flags: review?(dkeeler) → review+
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to David Keeler [:keeler] (use needinfo?) from comment #7) > If I'm reading this correctly, we now don't check that we've consumed all of > input (whereas I believe ExpectTagAndGetValueAtEnd did). (Ah, but I see that > BasicResponse is called from within a call to Nested, which does check that > we consume all of this input by the time we return, so I think this is ok.) Yes, exactly. I admit this isn't as clear as I'd like but this is a move towards making things more consistent which should ultimately provide that clarity. Thanks for the super-fast reviews!
Keywords: checkin-needed
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/438fa1ccf5fc https://hg.mozilla.org/integration/mozilla-inbound/rev/58fd93740f87 https://hg.mozilla.org/integration/mozilla-inbound/rev/37975b5017c4 https://hg.mozilla.org/integration/mozilla-inbound/rev/b71382534453
Keywords: checkin-needed
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/438fa1ccf5fc https://hg.mozilla.org/mozilla-central/rev/58fd93740f87 https://hg.mozilla.org/mozilla-central/rev/37975b5017c4 https://hg.mozilla.org/mozilla-central/rev/b71382534453
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•