Bug 1443744 - fix shadowing issues in pkix, r=keeler
authorFranziskus Kiefer <franziskuskiefer@gmail.com>
Wed, 07 Mar 2018 10:54:59 +0100
changeset 463695 2361015585d4b3c4e79f73831aa2ccd1d02dcb3f
parent 462592 d6ccaacd9692853a006461e883b4b9284c0e2964
child 463696 e7b736c64e87afa0cf39066f6956f11fadb1ce42
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskeeler
bugs1443744
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1443744 - fix shadowing issues in pkix, r=keeler Differential Revision: https://phabricator.services.mozilla.com/D689
security/pkix/include/pkix/Input.h
security/pkix/include/pkix/Time.h
security/pkix/include/pkix/pkixtypes.h
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixcheck.cpp
security/pkix/lib/pkixder.cpp
security/pkix/lib/pkixocsp.cpp
security/pkix/lib/pkixutil.h
security/pkix/moz.build
security/pkix/test/gtest/moz.build
security/pkix/test/gtest/pkixbuild_tests.cpp
security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp
security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
security/pkix/test/lib/pkixtestalg.cpp
security/pkix/test/lib/pkixtestnss.cpp
security/pkix/test/lib/pkixtestutil.cpp
security/pkix/test/lib/pkixtestutil.h
security/pkix/warnings.mozbuild
--- a/security/pkix/include/pkix/Input.h
+++ b/security/pkix/include/pkix/Input.h
@@ -61,47 +61,47 @@ public:
   //   const Input expected(EXPECTED_BYTES);
   //
   // This is equivalent to (and preferred over):
   //
   //   static const uint8_t EXPECTED_BYTES[] = { 0x00, 0x01, 0x02 };
   //   Input expected;
   //   Result rv = expected.Init(EXPECTED_BYTES, sizeof EXPECTED_BYTES);
   template <size_type N>
-  explicit Input(const uint8_t (&data)[N])
-    : data(data)
+  explicit Input(const uint8_t (&aData)[N])
+    : data(aData)
     , len(N)
   {
   }
 
   // Construct a valid, empty, Init-able Input.
   Input()
     : data(nullptr)
     , len(0u)
   {
   }
 
   // This is intentionally not explicit in order to allow value semantics.
   Input(const Input&) = default;
 
   // Initialize the input. data must be non-null and len must be less than
   // 65536. Init may not be called more than once.
-  Result Init(const uint8_t* data, size_t len)
+  Result Init(const uint8_t* aData, size_t aLen)
   {
     if (this->data) {
       // already initialized
       return Result::FATAL_ERROR_INVALID_ARGS;
     }
-    if (!data || len > 0xffffu) {
+    if (!aData || aLen > 0xffffu) {
       // input too large
       return Result::ERROR_BAD_DER;
     }
 
-    this->data = data;
-    this->len = len;
+    this->data = aData;
+    this->len = aLen;
 
     return Success;
   }
 
   // Initialize the input to be equivalent to the given input. Init may not be
   // called more than once.
   //
   // This is basically operator=, but it wasn't given that name because
@@ -148,29 +148,29 @@ class Reader final
 {
 public:
   Reader()
     : input(nullptr)
     , end(nullptr)
   {
   }
 
-  explicit Reader(Input input)
-    : input(input.UnsafeGetData())
-    , end(input.UnsafeGetData() + input.GetLength())
+  explicit Reader(Input aInput)
+    : input(aInput.UnsafeGetData())
+    , end(aInput.UnsafeGetData() + aInput.GetLength())
   {
   }
 
-  Result Init(Input input)
+  Result Init(Input aInput)
   {
     if (this->input) {
       return Result::FATAL_ERROR_INVALID_ARGS;
     }
-    this->input = input.UnsafeGetData();
-    this->end = input.UnsafeGetData() + input.GetLength();
+    this->input = aInput.UnsafeGetData();
+    this->end = aInput.UnsafeGetData() + aInput.GetLength();
     return Success;
   }
 
   bool Peek(uint8_t expectedByte) const
   {
     return input < end && *input == expectedByte;
   }
 
@@ -287,17 +287,17 @@ public:
   bool AtEnd() const { return input == end; }
 
   class Mark final
   {
   public:
     Mark(const Mark&) = default; // Intentionally not explicit.
   private:
     friend class Reader;
-    Mark(const Reader& input, const uint8_t* mark) : input(input), mark(mark) { }
+    Mark(const Reader& aInput, const uint8_t* aMark) : input(aInput), mark(aMark) { }
     const Reader& input;
     const uint8_t* const mark;
     void operator=(const Mark&) = delete;
   };
 
   Mark GetMark() const { return Mark(*this, input); }
 
   Result GetInput(const Mark& mark, /*out*/ Input& item)
--- a/security/pkix/include/pkix/Time.h
+++ b/security/pkix/include/pkix/Time.h
@@ -97,29 +97,29 @@ public:
 private:
   // This constructor is hidden to prevent accidents like this:
   //
   //    Time foo(time_t t)
   //    {
   //      // WRONG! 1970-01-01-00:00:00 == time_t(0), but not Time(0)!
   //      return Time(t);
   //    }
-  explicit Time(uint64_t elapsedSecondsAD)
-    : elapsedSecondsAD(elapsedSecondsAD)
+  explicit Time(uint64_t aElapsedSecondsAD)
+    : elapsedSecondsAD(aElapsedSecondsAD)
   {
   }
   friend Time TimeFromElapsedSecondsAD(uint64_t);
   friend class Duration;
 
   uint64_t elapsedSecondsAD;
 };
 
-inline Time TimeFromElapsedSecondsAD(uint64_t elapsedSecondsAD)
+inline Time TimeFromElapsedSecondsAD(uint64_t aElapsedSecondsAD)
 {
-  return Time(elapsedSecondsAD);
+  return Time(aElapsedSecondsAD);
 }
 
 Time Now();
 
 // Note the epoch is the unix epoch (ie 00:00:00 UTC, 1 January 1970)
 Time TimeFromEpochInSeconds(uint64_t secondsSinceEpoch);
 
 class Duration final
@@ -127,18 +127,18 @@ class Duration final
 public:
   Duration(Time timeA, Time timeB)
     : durationInSeconds(timeA < timeB
                         ? timeB.elapsedSecondsAD - timeA.elapsedSecondsAD
                         : timeA.elapsedSecondsAD - timeB.elapsedSecondsAD)
   {
   }
 
-  explicit Duration(uint64_t durationInSeconds)
-    : durationInSeconds(durationInSeconds)
+  explicit Duration(uint64_t aDurationInSeconds)
+    : durationInSeconds(aDurationInSeconds)
   {
   }
 
   bool operator>(const Duration& other) const
   {
     return durationInSeconds > other.durationInSeconds;
   }
   bool operator<(const Duration& other) const
--- a/security/pkix/include/pkix/pkixtypes.h
+++ b/security/pkix/include/pkix/pkixtypes.h
@@ -130,20 +130,20 @@ enum class AuxiliaryExtension
 // be encoded exactly the same, and the encoding matters for OCSP.)
 // issuerSubjectPublicKeyInfo is the entire DER-encoded subjectPublicKeyInfo
 // field from the issuer's certificate. serialNumber is the entire DER-encoded
 // serial number from the subject certificate (the certificate for which we are
 // checking the revocation status).
 struct CertID final
 {
 public:
-  CertID(Input issuer, Input issuerSubjectPublicKeyInfo, Input serialNumber)
-    : issuer(issuer)
-    , issuerSubjectPublicKeyInfo(issuerSubjectPublicKeyInfo)
-    , serialNumber(serialNumber)
+  CertID(Input aIssuer, Input aIssuerSubjectPublicKeyInfo, Input aSerialNumber)
+    : issuer(aIssuer)
+    , issuerSubjectPublicKeyInfo(aIssuerSubjectPublicKeyInfo)
+    , serialNumber(aSerialNumber)
   {
   }
   const Input issuer;
   const Input issuerSubjectPublicKeyInfo;
   const Input serialNumber;
 
   void operator=(const CertID&) = delete;
 };
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -41,29 +41,29 @@ static Result BuildForward(TrustDomain& 
 TrustDomain::IssuerChecker::IssuerChecker() { }
 TrustDomain::IssuerChecker::~IssuerChecker() { }
 
 // The implementation of TrustDomain::IssuerTracker is in a subclass only to
 // hide the implementation from external users.
 class PathBuildingStep final : public TrustDomain::IssuerChecker
 {
 public:
-  PathBuildingStep(TrustDomain& trustDomain, const BackCert& subject,
-                   Time time, KeyPurposeId requiredEKUIfPresent,
-                   const CertPolicyId& requiredPolicy,
-                   /*optional*/ const Input* stapledOCSPResponse,
-                   unsigned int subCACount, Result deferredSubjectError)
-    : trustDomain(trustDomain)
-    , subject(subject)
-    , time(time)
-    , requiredEKUIfPresent(requiredEKUIfPresent)
-    , requiredPolicy(requiredPolicy)
-    , stapledOCSPResponse(stapledOCSPResponse)
-    , subCACount(subCACount)
-    , deferredSubjectError(deferredSubjectError)
+  PathBuildingStep(TrustDomain& aTrustDomain, const BackCert& aSubject,
+                   Time aTime, KeyPurposeId aRequiredEKUIfPresent,
+                   const CertPolicyId& aRequiredPolicy,
+                   /*optional*/ const Input* aStapledOCSPResponse,
+                   unsigned int aSubCACount, Result aDeferredSubjectError)
+    : trustDomain(aTrustDomain)
+    , subject(aSubject)
+    , time(aTime)
+    , requiredEKUIfPresent(aRequiredEKUIfPresent)
+    , requiredPolicy(aRequiredPolicy)
+    , stapledOCSPResponse(aStapledOCSPResponse)
+    , subCACount(aSubCACount)
+    , deferredSubjectError(aDeferredSubjectError)
     , result(Result::FATAL_ERROR_LIBRARY_FAILURE)
     , resultWasSet(false)
   {
   }
 
   Result Check(Input potentialIssuerDER,
                /*optional*/ const Input* additionalNameConstraints,
                /*out*/ bool& keepGoing) override;
--- a/security/pkix/lib/pkixcheck.cpp
+++ b/security/pkix/lib/pkixcheck.cpp
@@ -334,25 +334,26 @@ CheckSubjectPublicKeyInfoContents(Reader
 
     // RSAPublicKey :: = SEQUENCE{
     //    modulus            INTEGER,    --n
     //    publicExponent     INTEGER  }  --e
     rv = der::Nested(subjectPublicKeyReader, der::SEQUENCE,
                      [&trustDomain, endEntityOrCA](Reader& r) {
       Input modulus;
       Input::size_type modulusSignificantBytes;
-      Result rv = der::PositiveInteger(r, modulus, &modulusSignificantBytes);
-      if (rv != Success) {
-        return rv;
+      Result nestedRv =
+        der::PositiveInteger(r, modulus, &modulusSignificantBytes);
+      if (nestedRv != Success) {
+        return nestedRv;
       }
       // XXX: Should we do additional checks of the modulus?
-      rv = trustDomain.CheckRSAPublicKeyModulusSizeInBits(
-             endEntityOrCA, modulusSignificantBytes * 8u);
-      if (rv != Success) {
-        return rv;
+      nestedRv = trustDomain.CheckRSAPublicKeyModulusSizeInBits(
+        endEntityOrCA, modulusSignificantBytes * 8u);
+      if (nestedRv != Success) {
+        return nestedRv;
       }
 
       // XXX: We don't allow the TrustDomain to validate the exponent.
       // XXX: We don't do our own sanity checking of the exponent.
       Input exponent;
       return der::PositiveInteger(r, exponent);
     });
     if (rv != Success) {
@@ -647,19 +648,19 @@ CheckBasicConstraints(EndEntityOrCA endE
 {
   bool isCA = false;
   long pathLenConstraint = UNLIMITED_PATH_LEN;
 
   if (encodedBasicConstraints) {
     Reader input(*encodedBasicConstraints);
     Result rv = der::Nested(input, der::SEQUENCE,
                             [&isCA, &pathLenConstraint](Reader& r) {
-      Result rv = der::OptionalBoolean(r, isCA);
-      if (rv != Success) {
-        return rv;
+      Result nestedRv = der::OptionalBoolean(r, isCA);
+      if (nestedRv != Success) {
+        return nestedRv;
       }
       // TODO(bug 985025): If isCA is false, pathLenConstraint
       // MUST NOT be included (as per RFC 5280 section
       // 4.2.1.9), but for compatibility reasons, we don't
       // check this.
       return der::OptionalInteger(r, UNLIMITED_PATH_LEN, pathLenConstraint);
     });
     if (rv != Success) {
--- a/security/pkix/lib/pkixder.cpp
+++ b/security/pkix/lib/pkixder.cpp
@@ -208,17 +208,16 @@ SignatureAlgorithmIdentifierValue(Reader
   }
 
   return Success;
 }
 
 Result
 DigestAlgorithmIdentifier(Reader& input, /*out*/ DigestAlgorithm& algorithm)
 {
-  Reader r;
   return der::Nested(input, SEQUENCE, [&algorithm](Reader& r) -> Result {
     Reader algorithmID;
     Result rv = AlgorithmIdentifierValue(r, algorithmID);
     if (rv != Success) {
       return rv;
     }
 
     // RFC 4055 Section 2.1
--- a/security/pkix/lib/pkixocsp.cpp
+++ b/security/pkix/lib/pkixocsp.cpp
@@ -41,26 +41,26 @@ enum class CertStatus : uint8_t {
   Good = der::CONTEXT_SPECIFIC | 0,
   Revoked = der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 1,
   Unknown = der::CONTEXT_SPECIFIC | 2
 };
 
 class Context final
 {
 public:
-  Context(TrustDomain& trustDomain, const CertID& certID, Time time,
-          uint16_t maxLifetimeInDays, /*optional out*/ Time* thisUpdate,
-          /*optional out*/ Time* validThrough)
-    : trustDomain(trustDomain)
-    , certID(certID)
-    , time(time)
-    , maxLifetimeInDays(maxLifetimeInDays)
+  Context(TrustDomain& aTrustDomain, const CertID& aCertID, Time aTime,
+          uint16_t aMaxLifetimeInDays, /*optional out*/ Time* aThisUpdate,
+          /*optional out*/ Time* aValidThrough)
+    : trustDomain(aTrustDomain)
+    , certID(aCertID)
+    , time(aTime)
+    , maxLifetimeInDays(aMaxLifetimeInDays)
     , certStatus(CertStatus::Unknown)
-    , thisUpdate(thisUpdate)
-    , validThrough(validThrough)
+    , thisUpdate(aThisUpdate)
+    , validThrough(aValidThrough)
     , expired(false)
     , matchFound(false)
   {
     if (thisUpdate) {
       *thisUpdate = TimeFromElapsedSecondsAD(0);
     }
     if (validThrough) {
       *validThrough = TimeFromElapsedSecondsAD(0);
@@ -168,19 +168,23 @@ static inline Result ResponseData(
                        const der::SignedDataWithSignature& signedResponseData,
                        const DERArray& certs);
 static inline Result SingleResponse(Reader& input, Context& context);
 static Result ExtensionNotUnderstood(Reader& extnID, Input extnValue,
                                      bool critical, /*out*/ bool& understood);
 static Result RememberSingleExtension(Context& context, Reader& extnID,
                                       Input extnValue, bool critical,
                                       /*out*/ bool& understood);
-static inline Result CertID(Reader& input,
-                            const Context& context,
-                            /*out*/ bool& match);
+// It is convention to name the function after the part of the data structure
+// we're parsing from the RFC (e.g. OCSPResponse, ResponseBytes).
+// But since we also have a C++ type called CertID, this function doesn't
+// follow the convention to prevent shadowing.
+static inline Result MatchCertID(Reader& input,
+                                 const Context& context,
+                                 /*out*/ bool& match);
 static Result MatchKeyHash(TrustDomain& trustDomain,
                            Input issuerKeyHash,
                            Input issuerSubjectPublicKeyInfo,
                            /*out*/ bool& match);
 static Result KeyHash(TrustDomain& trustDomain,
                       Input subjectPublicKeyInfo,
                       /*out*/ uint8_t* hashBuf, size_t hashBufSize);
 
@@ -433,22 +437,23 @@ BasicResponse(Reader& input, Context& co
 
   // Parse certificates, if any
   NonOwningDERArray certs;
   if (!input.AtEnd()) {
     rv = der::Nested(input, der::CONTEXT_SPECIFIC | der::CONSTRUCTED | 0,
                      der::SEQUENCE, [&certs](Reader& certsDER) -> Result {
       while (!certsDER.AtEnd()) {
         Input cert;
-        Result rv = der::ExpectTagAndGetTLV(certsDER, der::SEQUENCE, cert);
-        if (rv != Success) {
-          return rv;
+        Result nestedRv =
+          der::ExpectTagAndGetTLV(certsDER, der::SEQUENCE, cert);
+        if (nestedRv != Success) {
+          return nestedRv;
         }
-        rv = certs.Append(cert);
-        if (rv != Success) {
+        nestedRv = certs.Append(cert);
+        if (nestedRv != Success) {
           return Result::ERROR_BAD_DER; // Too many certs
         }
       }
       return Success;
     });
     if (rv != Success) {
       return rv;
     }
@@ -533,17 +538,17 @@ ResponseData(Reader& input, Context& con
 //                                              re-ocsp-archive-cutoff |
 //                                              CrlEntryExtensions, ...}
 //                                              } OPTIONAL }
 static inline Result
 SingleResponse(Reader& input, Context& context)
 {
   bool match = false;
   Result rv = der::Nested(input, der::SEQUENCE, [&context, &match](Reader& r) {
-    return CertID(r, context, match);
+    return MatchCertID(r, context, match);
   });
   if (rv != Success) {
     return rv;
   }
 
   if (!match) {
     // This response does not reference the certificate we're interested in.
     // By consuming the rest of our input and returning successfully, we can
@@ -690,17 +695,17 @@ SingleResponse(Reader& input, Context& c
 }
 
 // CertID          ::=     SEQUENCE {
 //        hashAlgorithm       AlgorithmIdentifier,
 //        issuerNameHash      OCTET STRING, -- Hash of issuer's DN
 //        issuerKeyHash       OCTET STRING, -- Hash of issuer's public key
 //        serialNumber        CertificateSerialNumber }
 static inline Result
-CertID(Reader& input, const Context& context, /*out*/ bool& match)
+MatchCertID(Reader& input, const Context& context, /*out*/ bool& match)
 {
   match = false;
 
   DigestAlgorithm hashAlgorithm;
   Result rv = der::DigestAlgorithmIdentifier(input, hashAlgorithm);
   if (rv != Success) {
     if (rv == Result::ERROR_INVALID_ALGORITHM) {
       // Skip entries that are hashed with algorithms we don't support.
--- a/security/pkix/lib/pkixutil.h
+++ b/security/pkix/lib/pkixutil.h
@@ -37,21 +37,21 @@ namespace mozilla { namespace pkix {
 //
 // Each BackCert contains pointers to all the given certificate's extensions
 // so that we can parse the extension block once and then process the
 // extensions in an order that may be different than they appear in the cert.
 class BackCert final
 {
 public:
   // certDER and childCert must be valid for the lifetime of BackCert.
-  BackCert(Input certDER, EndEntityOrCA endEntityOrCA,
-           const BackCert* childCert)
-    : der(certDER)
-    , endEntityOrCA(endEntityOrCA)
-    , childCert(childCert)
+  BackCert(Input aCertDER, EndEntityOrCA aEndEntityOrCA,
+           const BackCert* aChildCert)
+    : der(aCertDER)
+    , endEntityOrCA(aEndEntityOrCA)
+    , childCert(aChildCert)
   {
   }
 
   Result Init();
 
   const Input GetDER() const { return der; }
   const der::SignedDataWithSignature& GetSignedData() const {
     return signedData;
--- a/security/pkix/moz.build
+++ b/security/pkix/moz.build
@@ -29,16 +29,8 @@ TEST_DIRS += [
     'test/lib',
 ]
 
 include('warnings.mozbuild')
 
 Library('mozillapkix')
 
 FINAL_LIBRARY = 'xul'
-
-if CONFIG['CC_TYPE'] in ('clang', 'gcc'):
-    CXXFLAGS += ['-Wno-error=shadow']
-
-if CONFIG['CC_TYPE'] in ('msvc', 'clang-cl'):
-    # This is intended as a temporary hack to support building with VS2015.
-    # declaration of '*' hides class member
-    CXXFLAGS += ['-wd4458']
--- a/security/pkix/test/gtest/moz.build
+++ b/security/pkix/test/gtest/moz.build
@@ -42,17 +42,16 @@ include('../../warnings.mozbuild')
 if CONFIG['CC_TYPE'] == 'gcc':
   CXXFLAGS.remove('-pedantic-errors')
 
 # These warnings are disabled in order to minimize the amount of boilerplate
 # required to implement tests, and/or because they originate in the GTest
 # framework in a way we cannot otherwise work around.
 if CONFIG['CC_TYPE'] in ('clang', 'gcc'):
   CXXFLAGS += [
-    '-Wno-error=shadow',
     '-Wno-old-style-cast',
   ]
   if CONFIG['CC_TYPE'] == 'clang':
     CXXFLAGS += [
       '-Wno-exit-time-destructors',
       '-Wno-global-constructors',
       '-Wno-thread-safety',
       '-Wno-used-but-marked-unused',
--- a/security/pkix/test/gtest/pkixbuild_tests.cpp
+++ b/security/pkix/test/gtest/pkixbuild_tests.cpp
@@ -250,18 +250,18 @@ TEST_F(pkixbuild, BeyondMaxAcceptableCer
 // A TrustDomain that checks certificates against a given root certificate.
 // It is initialized with the DER encoding of a root certificate that
 // is treated as a trust anchor and is assumed to have issued all certificates
 // (i.e. FindIssuer always attempts to build the next step in the chain with
 // it).
 class SingleRootTrustDomain : public DefaultCryptoTrustDomain
 {
 public:
-  explicit SingleRootTrustDomain(ByteString rootDER)
-    : rootDER(rootDER)
+  explicit SingleRootTrustDomain(ByteString aRootDER)
+    : rootDER(aRootDER)
   {
   }
 
   // The CertPolicyId argument is unused because we don't care about EV.
   Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
                       /*out*/ TrustLevel& trustLevel) override
   {
     Input rootCert;
@@ -306,18 +306,18 @@ public:
 private:
   ByteString rootDER;
 };
 
 // A TrustDomain that explicitly fails if CheckRevocation is called.
 class ExpiredCertTrustDomain final : public SingleRootTrustDomain
 {
 public:
-  explicit ExpiredCertTrustDomain(ByteString rootDER)
-    : SingleRootTrustDomain(rootDER)
+  explicit ExpiredCertTrustDomain(ByteString aRootDER)
+    : SingleRootTrustDomain(aRootDER)
   {
   }
 
   Result CheckRevocation(EndEntityOrCA, const CertID&, Time, Duration,
                          /*optional*/ const Input*, /*optional*/ const Input*)
                          override
   {
     ADD_FAILURE();
@@ -405,19 +405,19 @@ TEST_F(pkixbuild_DSS, DSSEndEntityKeyNot
                            KeyPurposeId::id_kp_serverAuth,
                            CertPolicyId::anyPolicy,
                            nullptr/*stapledOCSPResponse*/));
 }
 
 class IssuerNameCheckTrustDomain final : public DefaultCryptoTrustDomain
 {
 public:
-  IssuerNameCheckTrustDomain(const ByteString& issuer, bool expectedKeepGoing)
-    : issuer(issuer)
-    , expectedKeepGoing(expectedKeepGoing)
+  IssuerNameCheckTrustDomain(const ByteString& aIssuer, bool aExpectedKeepGoing)
+    : issuer(aIssuer)
+    , expectedKeepGoing(aExpectedKeepGoing)
   {
   }
 
   Result GetCertTrust(EndEntityOrCA endEntityOrCA, const CertPolicyId&, Input,
                       /*out*/ TrustLevel& trustLevel) override
   {
     trustLevel = endEntityOrCA == EndEntityOrCA::MustBeCA
                ? TrustLevel::TrustAnchor
@@ -515,18 +515,18 @@ TEST_P(pkixbuild_IssuerNameCheck, Matchi
 INSTANTIATE_TEST_CASE_P(pkixbuild_IssuerNameCheck, pkixbuild_IssuerNameCheck,
                         testing::ValuesIn(ISSUER_NAME_CHECK_PARAMS));
 
 
 // Records the embedded SCT list extension for later examination.
 class EmbeddedSCTListTestTrustDomain final : public SingleRootTrustDomain
 {
 public:
-  explicit EmbeddedSCTListTestTrustDomain(ByteString rootDER)
-    : SingleRootTrustDomain(rootDER)
+  explicit EmbeddedSCTListTestTrustDomain(ByteString aRootDER)
+    : SingleRootTrustDomain(aRootDER)
   {
   }
 
   virtual void NoteAuxiliaryExtension(AuxiliaryExtension extension,
                                       Input extensionData) override
   {
     if (extension == AuxiliaryExtension::EmbeddedSCTList) {
       signedCertificateTimestamps = InputToByteString(extensionData);
--- a/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp
+++ b/security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp
@@ -42,24 +42,24 @@ CreateCert(const char* issuerCN,
                                               signatureAlgorithm));
   EXPECT_FALSE(ENCODING_FAILED(certDER));
   return certDER;
 }
 
 class AlgorithmTestsTrustDomain final : public DefaultCryptoTrustDomain
 {
 public:
-  AlgorithmTestsTrustDomain(const ByteString& rootDER,
-                            const ByteString& rootSubjectDER,
-               /*optional*/ const ByteString& intDER,
-               /*optional*/ const ByteString& intSubjectDER)
-    : rootDER(rootDER)
-    , rootSubjectDER(rootSubjectDER)
-    , intDER(intDER)
-    , intSubjectDER(intSubjectDER)
+  AlgorithmTestsTrustDomain(const ByteString& aRootDER,
+                            const ByteString& aRootSubjectDER,
+               /*optional*/ const ByteString& aIntDER,
+               /*optional*/ const ByteString& aIntSubjectDER)
+    : rootDER(aRootDER)
+    , rootSubjectDER(aRootSubjectDER)
+    , intDER(aIntDER)
+    , intSubjectDER(aIntSubjectDER)
   {
   }
 
 private:
   Result GetCertTrust(EndEntityOrCA, const CertPolicyId&, Input candidateCert,
                       /*out*/ TrustLevel& trustLevel) override
   {
     if (InputEqualsByteString(candidateCert, rootDER)) {
@@ -113,24 +113,24 @@ static const TestSignatureAlgorithm NO_I
   TestPublicKeyAlgorithm(ByteString()),
   TestDigestAlgorithmID::MD2,
   ByteString(),
   false
 };
 
 struct ChainValidity final
 {
-  ChainValidity(const TestSignatureAlgorithm& endEntitySignatureAlgorithm,
-                const TestSignatureAlgorithm& optionalIntSignatureAlgorithm,
-                const TestSignatureAlgorithm& rootSignatureAlgorithm,
-                bool isValid)
-    : endEntitySignatureAlgorithm(endEntitySignatureAlgorithm)
-    , optionalIntermediateSignatureAlgorithm(optionalIntSignatureAlgorithm)
-    , rootSignatureAlgorithm(rootSignatureAlgorithm)
-    , isValid(isValid)
+  ChainValidity(const TestSignatureAlgorithm& aEndEntitySignatureAlgorithm,
+                const TestSignatureAlgorithm& aOptionalIntSignatureAlgorithm,
+                const TestSignatureAlgorithm& aRootSignatureAlgorithm,
+                bool aIsValid)
+    : endEntitySignatureAlgorithm(aEndEntitySignatureAlgorithm)
+    , optionalIntermediateSignatureAlgorithm(aOptionalIntSignatureAlgorithm)
+    , rootSignatureAlgorithm(aRootSignatureAlgorithm)
+    , isValid(aIsValid)
   { }
 
   // In general, a certificate is generated for each of these.  However, if
   // optionalIntermediateSignatureAlgorithm is NO_INTERMEDIATE, then only 2
   // certificates are generated.
   // The certificate generated for the given rootSignatureAlgorithm is the
   // trust anchor.
   TestSignatureAlgorithm endEntitySignatureAlgorithm;
--- a/security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
+++ b/security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp
@@ -198,18 +198,18 @@ class pkixcheck_CheckSignatureAlgorithm
 {
 };
 
 class pkixcheck_CheckSignatureAlgorithm_TrustDomain final
   : public EverythingFailsByDefaultTrustDomain
 {
 public:
   explicit pkixcheck_CheckSignatureAlgorithm_TrustDomain(
-             unsigned int publicKeySizeInBits)
-    : publicKeySizeInBits(publicKeySizeInBits)
+             unsigned int aPublicKeySizeInBits)
+    : publicKeySizeInBits(aPublicKeySizeInBits)
     , checkedDigestAlgorithm(false)
     , checkedModulusSizeInBits(false)
   {
   }
 
   Result CheckSignatureDigestAlgorithm(DigestAlgorithm, EndEntityOrCA, Time)
     override
   {
@@ -268,18 +268,18 @@ INSTANTIATE_TEST_CASE_P(
   pkixcheck_CheckSignatureAlgorithm, pkixcheck_CheckSignatureAlgorithm,
   testing::ValuesIn(CHECKSIGNATUREALGORITHM_TEST_PARAMS));
 
 class pkixcheck_CheckSignatureAlgorithm_BuildCertChain_TrustDomain
   : public DefaultCryptoTrustDomain
 {
 public:
   explicit pkixcheck_CheckSignatureAlgorithm_BuildCertChain_TrustDomain(
-             const ByteString& issuer)
-    : issuer(issuer)
+             const ByteString& aIssuer)
+    : issuer(aIssuer)
   {
   }
 
   Result GetCertTrust(EndEntityOrCA, const CertPolicyId&,
                       Input cert, /*out*/ TrustLevel& trustLevel) override
   {
     trustLevel = InputEqualsByteString(cert, issuer)
                ? TrustLevel::TrustAnchor
--- a/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
+++ b/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ -972,20 +972,20 @@ public:
   class TrustDomain final : public OCSPTestTrustDomain
   {
   public:
     TrustDomain()
       : certTrustLevel(TrustLevel::InheritsTrust)
     {
     }
 
-    bool SetCertTrust(const ByteString& certDER, TrustLevel certTrustLevel)
+    bool SetCertTrust(const ByteString& aCertDER, TrustLevel aCertTrustLevel)
     {
-      this->certDER = certDER;
-      this->certTrustLevel = certTrustLevel;
+      this->certDER = aCertDER;
+      this->certTrustLevel = aCertTrustLevel;
       return true;
     }
   private:
     Result GetCertTrust(EndEntityOrCA endEntityOrCA, const CertPolicyId&,
                         Input candidateCert, /*out*/ TrustLevel& trustLevel)
                         override
     {
       EXPECT_EQ(endEntityOrCA, EndEntityOrCA::MustBeEndEntity);
--- a/security/pkix/test/lib/pkixtestalg.cpp
+++ b/security/pkix/test/lib/pkixtestalg.cpp
@@ -132,24 +132,24 @@ static const uint8_t DSS_G_RAW[] =
   0x76,0x1F,0x25,0x43,0xB6,0xDE,0x56,0xF7,0x52,0xCC,0x07,0xB8,
   0x37,0xE2,0x8C,0xC5,0x56,0x8C,0xDD,0x63,0xF5,0xB6,0xA3,0x46,
   0x62,0xF6,0x35,0x76,
 };
 
 } // namespace
 
 TestSignatureAlgorithm::TestSignatureAlgorithm(
-  const TestPublicKeyAlgorithm& publicKeyAlg,
-  TestDigestAlgorithmID digestAlg,
-  const ByteString& algorithmIdentifier,
-  bool accepted)
-  : publicKeyAlg(publicKeyAlg)
-  , digestAlg(digestAlg)
-  , algorithmIdentifier(algorithmIdentifier)
-  , accepted(accepted)
+  const TestPublicKeyAlgorithm& aPublicKeyAlg,
+  TestDigestAlgorithmID aDigestAlg,
+  const ByteString& aAlgorithmIdentifier,
+  bool aAccepted)
+  : publicKeyAlg(aPublicKeyAlg)
+  , digestAlg(aDigestAlg)
+  , algorithmIdentifier(aAlgorithmIdentifier)
+  , accepted(aAccepted)
 {
 }
 
 ByteString DSS_P() { return ByteString(DSS_P_RAW, sizeof(DSS_P_RAW)); }
 ByteString DSS_Q() { return ByteString(DSS_Q_RAW, sizeof(DSS_Q_RAW)); }
 ByteString DSS_G() { return ByteString(DSS_G_RAW, sizeof(DSS_G_RAW)); }
 
 TestPublicKeyAlgorithm
--- a/security/pkix/test/lib/pkixtestnss.cpp
+++ b/security/pkix/test/lib/pkixtestnss.cpp
@@ -75,25 +75,25 @@ InitReusedKeyPair()
   InitNSSIfNeeded();
   reusedKeyPair.reset(GenerateKeyPairInner());
   return reusedKeyPair ? PR_SUCCESS : PR_FAILURE;
 }
 
 class NSSTestKeyPair final : public TestKeyPair
 {
 public:
-  NSSTestKeyPair(const TestPublicKeyAlgorithm& publicKeyAlg,
+  NSSTestKeyPair(const TestPublicKeyAlgorithm& aPublicKeyAlg,
                  const ByteString& spk,
-                 const ByteString& encryptedPrivateKey,
-                 const ByteString& encryptionAlgorithm,
-                 const ByteString& encryptionParams)
-    : TestKeyPair(publicKeyAlg, spk)
-    , encryptedPrivateKey(encryptedPrivateKey)
-    , encryptionAlgorithm(encryptionAlgorithm)
-    , encryptionParams(encryptionParams)
+                 const ByteString& aEncryptedPrivateKey,
+                 const ByteString& aEncryptionAlgorithm,
+                 const ByteString& aEncryptionParams)
+    : TestKeyPair(aPublicKeyAlg, spk)
+    , encryptedPrivateKey(aEncryptedPrivateKey)
+    , encryptionAlgorithm(aEncryptionAlgorithm)
+    , encryptionParams(aEncryptionParams)
   {
   }
 
   Result SignData(const ByteString& tbs,
                   const TestSignatureAlgorithm& signatureAlgorithm,
                   /*out*/ ByteString& signature) const override
   {
     SECOidTag oidTag;
--- a/security/pkix/test/lib/pkixtestutil.cpp
+++ b/security/pkix/test/lib/pkixtestutil.cpp
@@ -149,18 +149,18 @@ TLV(uint8_t tag, size_t length, const By
 OCSPResponseExtension::OCSPResponseExtension()
   : id()
   , critical(false)
   , value()
   , next(nullptr)
 {
 }
 
-OCSPResponseContext::OCSPResponseContext(const CertID& certID, time_t time)
-  : certID(certID)
+OCSPResponseContext::OCSPResponseContext(const CertID& aCertID, time_t time)
+  : certID(aCertID)
   , responseStatus(successful)
   , skipResponseBytes(false)
   , producedAt(time)
   , singleExtensions(nullptr)
   , responseExtensions(nullptr)
   , includeEmptyExtensions(false)
   , signatureAlgorithm(sha256WithRSAEncryption())
   , badSignature(false)
@@ -1137,19 +1137,19 @@ CertStatus(OCSPResponseContext& context)
       // fall through
   }
   return ByteString();
 }
 
 static const ByteString NO_UNUSED_BITS(1, 0x00);
 
 // The SubjectPublicKeyInfo syntax is specified in RFC 5280 Section 4.1.
-TestKeyPair::TestKeyPair(const TestPublicKeyAlgorithm& publicKeyAlg,
+TestKeyPair::TestKeyPair(const TestPublicKeyAlgorithm& aPublicKeyAlg,
                          const ByteString& spk)
-  : publicKeyAlg(publicKeyAlg)
+  : publicKeyAlg(aPublicKeyAlg)
   , subjectPublicKeyInfo(TLV(der::SEQUENCE,
-                             publicKeyAlg.algorithmIdentifier +
+                             aPublicKeyAlg.algorithmIdentifier +
                              TLV(der::BIT_STRING, NO_UNUSED_BITS + spk)))
   , subjectPublicKey(spk)
 {
 }
 
 } } } // namespace mozilla::pkix::test
--- a/security/pkix/test/lib/pkixtestutil.h
+++ b/security/pkix/test/lib/pkixtestutil.h
@@ -82,18 +82,18 @@ enum class TestDigestAlgorithmID
   SHA224,
   SHA256,
   SHA384,
   SHA512,
 };
 
 struct TestPublicKeyAlgorithm
 {
-  explicit TestPublicKeyAlgorithm(const ByteString& algorithmIdentifier)
-    : algorithmIdentifier(algorithmIdentifier) { }
+  explicit TestPublicKeyAlgorithm(const ByteString& aAlgorithmIdentifier)
+    : algorithmIdentifier(aAlgorithmIdentifier) { }
   bool operator==(const TestPublicKeyAlgorithm& other) const
   {
     return algorithmIdentifier == other.algorithmIdentifier;
   }
   ByteString algorithmIdentifier;
 };
 
 ByteString DSS_P();
--- a/security/pkix/warnings.mozbuild
+++ b/security/pkix/warnings.mozbuild
@@ -3,17 +3,16 @@ if CONFIG['CC_TYPE'] == 'clang':
     '-Weverything',
 
     '-Wno-c++98-compat',
     '-Wno-c++98-compat-pedantic',
     '-Wno-missing-prototypes',
     '-Wno-missing-variable-declarations',
     '-Wno-padded',
     '-Wno-reserved-id-macro', # NSPR and NSS use reserved IDs in their include guards.
-    '-Wno-shadow', # XXX: Clang's rules are too strict for constructors.
     '-Wno-weak-vtables', # We rely on the linker to merge the duplicate vtables.
   ]
 elif CONFIG['CC_TYPE'] in ('msvc', 'clang-cl'):
   CXXFLAGS += [
     '-sdl', # Enable additional security checks based on Microsoft's SDL.
 
     '-Wall',