1002933 - Use strongly-typed enums more often in mozilla::pkix

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: enum-classes.patch

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.

Comment 1

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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

[[:mmc] Monica Chew (no longer reading bugmail)]

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.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c968e47ef708

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c968e47ef708

Comment 6

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: backport-MOZILLA_PKIX_ENUM_CLASS-Beta.patch

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

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/releases/mozilla-beta/rev/345ff784e85e