Closed
Bug 1002933
Opened 10 years ago
Closed 10 years ago
Use strongly-typed enums more often in mozilla::pkix
Categories
(Core :: Security: PSM, enhancement)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: briansmith, Assigned: briansmith)
Details
Attachments
(2 files)
46.21 KB,
patch
|
mmc
:
review-
|
Details | Diff | Splinter Review |
2.54 KB,
patch
|
briansmith
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
C++11 strongly-typed enum classes prevent automatic conversion of an enum value from one enum type to another enum type, and they prevent automatic version from integer types (including boolean) to/from enum values. Thus, using enum classes instead of regular enums makes the code more type-safe and thus makes it harder to write wrong code. I did not change all enums to be enum classes. Eventually mozilla::pkix::Result will become an enum class, but that is a large change that will happen in its own bug. similarly, der::Version should become an enum class but I have some patches that may make der::Version completely unnecessary.
Attachment #8414237 -
Flags: review?(mmc)
Comment 1•10 years ago
|
||
Comment on attachment 8414237 [details] [diff] [review] enum-classes.patch Review of attachment 8414237 [details] [diff] [review]: ----------------------------------------------------------------- LG pending a green try push. Yours looks broken on unsigned/signed comparisons, but perhaps that's another patch in your push? https://tbpl.mozilla.org/php/getParsedLog.php?id=38680036&tree=Try ::: security/pkix/include/pkix/pkixtypes.h @@ +35,5 @@ > ScopedSECKEYPublicKey; > > typedef unsigned int KeyUsages; > > +MOZILLA_PKIX_ENUM_CLASS EndEntityOrCA { MustBeEndEntity, MustBeCA }; Go ahead and assign values to be consistent. ::: security/pkix/lib/pkixcheck.cpp @@ +50,5 @@ > // TODO(bug 970196): Reject certificates that are being used to verify > // certificate signatures unless the certificate is a trust anchor, to > // reduce the chances of an end-entity certificate being abused as a CA > // certificate. > + // if (endEntityOrCA == EndEntityOrCA::MustBeCA && !isTrustAnchor) { Normally it's safer for non-lvalues to be on the left just in case someone mistypes == as =, but I see this style is used throughout.
Updated•10 years ago
|
Attachment #8414237 -
Flags: review?(mmc) → review-
Assignee | ||
Comment 2•10 years ago
|
||
(In reply to [:mmc] Monica Chew (please use needinfo) from comment #1) > LG pending a green try push. Yours looks broken on unsigned/signed > comparisons, but perhaps that's another patch in your push? > > https://tbpl.mozilla.org/php/getParsedLog.php?id=38680036&tree=Try https://tbpl.mozilla.org/?tree=Try&rev=8df5acc26dee The log you linked to was for an earlier push that tried to enable FAIL_ON_WARNINGS in security/pkix/test/gtest; I removed that changeset. > > +MOZILLA_PKIX_ENUM_CLASS EndEntityOrCA { MustBeEndEntity, MustBeCA }; > > Go ahead and assign values to be consistent. OK. I also changed -MOZILLA_PKIX_ENUM_CLASS EmptyAllowed { Yes = 0, No = 1 }; +MOZILLA_PKIX_ENUM_CLASS EmptyAllowed { No = 0, Yes = 1 }; to be consistent. > Normally it's safer for non-lvalues to be on the left just in case someone > mistypes == as =, but I see this style is used throughout. I agree, but that's not the normal Gecko style. Also, I have a patch queued which will enable the compiler warnings to catch that type of thing.
Comment 3•10 years ago
|
||
r=me with changes you mentioned. > OK. I also changed > > -MOZILLA_PKIX_ENUM_CLASS EmptyAllowed { Yes = 0, No = 1 }; > +MOZILLA_PKIX_ENUM_CLASS EmptyAllowed { No = 0, Yes = 1 }; > > to be consistent. OK, great. That also catches implicit conversion better. > > Normally it's safer for non-lvalues to be on the left just in case someone > > mistypes == as =, but I see this style is used throughout. > > I agree, but that's not the normal Gecko style. Also, I have a patch queued > which will enable the compiler warnings to catch that type of thing. Sweet, I hope it lands soon.
Assignee | ||
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c968e47ef708
Target Milestone: --- → mozilla32
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c968e47ef708
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 6•10 years ago
|
||
This is a subset of the patch already reviewed by mmc. Approval Request Comment [Feature/regressing bug #]: bug 915930 [User impact if declined]: null pointer dereference due to not being able to backport the fix for 1031542. [Describe test coverage new/current, TBPL]: This is tested extensively using the tests in security/pkix/test/gtest. I will backport the necessary tests along with the patch. [Risks and why]: Very low risk. This code has been working fine in tests and on Mozilla-Central and Mozilla-Aurora for several weeks. [String/UUID change made/needed]: none
Attachment #8447583 -
Flags: review+
Attachment #8447583 -
Flags: approval-mozilla-beta?
Updated•10 years ago
|
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8447583 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/345ff784e85e
You need to log in
before you can comment on or make changes to this bug.
Description
•