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)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- fixed
firefox32 --- fixed
firefox33 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

Details

Attachments

(2 files)

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 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.
Attachment #8414237 - Flags: review?(mmc) → review-
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/c968e47ef708
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
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?
Attachment #8447583 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.