Bug 1443744 - fix shadowing issues in pkix, r=keeler

Differential Revision: https://phabricator.services.mozilla.com/D689

security/pkix/include/pkix/Input.h

@@ -66,8 +66,8 @@ public:
   //   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)
   {
   }
@@ -84,19 +84,19 @@ public:
 
   // 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;
   }
@@ -153,19 +153,19 @@ public:
   {
   }
 
-  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;
   }
 
@@ -292,7 +292,7 @@ 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;

security/pkix/include/pkix/Time.h

@@ -102,8 +102,8 @@ private:
   //      // 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);
@@ -112,9 +112,9 @@ private:
   uint64_t elapsedSecondsAD;
 };
 
-inline Time TimeFromElapsedSecondsAD(uint64_t elapsedSecondsAD)
+inline Time TimeFromElapsedSecondsAD(uint64_t aElapsedSecondsAD)
 {
-  return Time(elapsedSecondsAD);
+  return Time(aElapsedSecondsAD);
 }
 
 Time Now();
@@ -132,8 +132,8 @@ public:
   {
   }
 
-  explicit Duration(uint64_t durationInSeconds)
-    : durationInSeconds(durationInSeconds)
+  explicit Duration(uint64_t aDurationInSeconds)
+    : durationInSeconds(aDurationInSeconds)
   {
   }
 

security/pkix/include/pkix/pkixtypes.h

@@ -135,10 +135,10 @@ enum class AuxiliaryExtension
 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;

security/pkix/lib/pkixbuild.cpp

@@ -46,19 +46,19 @@ TrustDomain::IssuerChecker::~IssuerChecker() { }
 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)
   {

security/pkix/lib/pkixcheck.cpp

@@ -339,15 +339,16 @@ CheckSubjectPublicKeyInfoContents(Reader& input, TrustDomain& trustDomain,
                      [&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(
+      nestedRv = trustDomain.CheckRSAPublicKeyModulusSizeInBits(
         endEntityOrCA, modulusSignificantBytes * 8u);
-      if (rv != Success) {
-        return rv;
+      if (nestedRv != Success) {
+        return nestedRv;
       }
 
       // XXX: We don't allow the TrustDomain to validate the exponent.
@@ -652,9 +653,9 @@ CheckBasicConstraints(EndEntityOrCA endEntityOrCA,
     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

security/pkix/lib/pkixder.cpp

@@ -213,7 +213,6 @@ SignatureAlgorithmIdentifierValue(Reader& input,
 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);

security/pkix/lib/pkixocsp.cpp

@@ -46,16 +46,16 @@ enum class CertStatus : uint8_t {
 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)
   {
@@ -173,7 +173,11 @@ static Result ExtensionNotUnderstood(Reader& extnID, Input extnValue,
 static Result RememberSingleExtension(Context& context, Reader& extnID,
                                       Input extnValue, bool critical,
                                       /*out*/ bool& understood);
-static inline Result CertID(Reader& input,
+// 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,
@@ -438,12 +442,13 @@ BasicResponse(Reader& input, Context& context)
                      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
         }
       }
@@ -538,7 +543,7 @@ 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;
@@ -695,7 +700,7 @@ SingleResponse(Reader& input, Context& context)
 //        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;
 

security/pkix/lib/pkixutil.h

@@ -42,11 +42,11 @@ 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)
   {
   }
 

security/pkix/moz.build

@@ -34,11 +34,3 @@ 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']

security/pkix/test/gtest/moz.build

@@ -47,7 +47,6 @@ if CONFIG['CC_TYPE'] == 'gcc':
 # 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':

security/pkix/test/gtest/pkixbuild_tests.cpp

@@ -255,8 +255,8 @@ TEST_F(pkixbuild, BeyondMaxAcceptableCertChainLength)
 class SingleRootTrustDomain : public DefaultCryptoTrustDomain
 {
 public:
-  explicit SingleRootTrustDomain(ByteString rootDER)
-    : rootDER(rootDER)
+  explicit SingleRootTrustDomain(ByteString aRootDER)
+    : rootDER(aRootDER)
   {
   }
 
@@ -311,8 +311,8 @@ private:
 class ExpiredCertTrustDomain final : public SingleRootTrustDomain
 {
 public:
-  explicit ExpiredCertTrustDomain(ByteString rootDER)
-    : SingleRootTrustDomain(rootDER)
+  explicit ExpiredCertTrustDomain(ByteString aRootDER)
+    : SingleRootTrustDomain(aRootDER)
   {
   }
 
@@ -410,9 +410,9 @@ TEST_F(pkixbuild_DSS, DSSEndEntityKeyNotAccepted)
 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)
   {
   }
 
@@ -520,8 +520,8 @@ INSTANTIATE_TEST_CASE_P(pkixbuild_IssuerNameCheck, pkixbuild_IssuerNameCheck,
 class EmbeddedSCTListTestTrustDomain final : public SingleRootTrustDomain
 {
 public:
-  explicit EmbeddedSCTListTestTrustDomain(ByteString rootDER)
-    : SingleRootTrustDomain(rootDER)
+  explicit EmbeddedSCTListTestTrustDomain(ByteString aRootDER)
+    : SingleRootTrustDomain(aRootDER)
   {
   }
 

security/pkix/test/gtest/pkixcert_signature_algorithm_tests.cpp

@@ -47,14 +47,14 @@ CreateCert(const char* issuerCN,
 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)
   {
   }
 
@@ -118,14 +118,14 @@ static const TestSignatureAlgorithm NO_INTERMEDIATE
 
 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

security/pkix/test/gtest/pkixcheck_CheckSignatureAlgorithm_tests.cpp

@@ -203,8 +203,8 @@ class pkixcheck_CheckSignatureAlgorithm_TrustDomain final
 {
 public:
   explicit pkixcheck_CheckSignatureAlgorithm_TrustDomain(
-             unsigned int publicKeySizeInBits)
-    : publicKeySizeInBits(publicKeySizeInBits)
+             unsigned int aPublicKeySizeInBits)
+    : publicKeySizeInBits(aPublicKeySizeInBits)
     , checkedDigestAlgorithm(false)
     , checkedModulusSizeInBits(false)
   {
@@ -273,8 +273,8 @@ class pkixcheck_CheckSignatureAlgorithm_BuildCertChain_TrustDomain
 {
 public:
   explicit pkixcheck_CheckSignatureAlgorithm_BuildCertChain_TrustDomain(
-             const ByteString& issuer)
-    : issuer(issuer)
+             const ByteString& aIssuer)
+    : issuer(aIssuer)
   {
   }
 

security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp

@@ -977,10 +977,10 @@ public:
     {
     }
 
-    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:

security/pkix/test/lib/pkixtestalg.cpp

@@ -137,14 +137,14 @@ static const uint8_t DSS_G_RAW[] =
 } // 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)
 {
 }
 

security/pkix/test/lib/pkixtestnss.cpp

@@ -80,15 +80,15 @@ InitReusedKeyPair()
 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)
   {
   }
 

security/pkix/test/lib/pkixtestutil.cpp

@@ -154,8 +154,8 @@ OCSPResponseExtension::OCSPResponseExtension()
 {
 }
 
-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)
@@ -1142,11 +1142,11 @@ CertStatus(OCSPResponseContext& context)
 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)
 {

security/pkix/test/lib/pkixtestutil.h

@@ -87,8 +87,8 @@ enum class TestDigestAlgorithmID
 
 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;

security/pkix/warnings.mozbuild

@@ -8,7 +8,6 @@ if CONFIG['CC_TYPE'] == 'clang':
     '-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'):