Bug 1000544: Use "Fail(x, y)" instead of "PR_SetError(y, 0); return x;" more consistently, r=mmc
authorBrian Smith <brian@briansmith.org>
Wed, 23 Apr 2014 14:13:32 -0700
changeset 181061 146e4f4e985c89f2ace6a7150844a430349a9348
parent 181060 617e8978be14dc6c56d820bd5f85653d954404b8
child 181062 64ce640f432e28a628b2a21ff7e392a475e87f0c
push id272
push userpvanderbeken@mozilla.com
push dateMon, 05 May 2014 16:31:18 +0000
reviewersmmc
bugs1000544
milestone32.0a1
Bug 1000544: Use "Fail(x, y)" instead of "PR_SetError(y, 0); return x;" more consistently, r=mmc
security/pkix/lib/pkixbuild.cpp
security/pkix/lib/pkixcheck.cpp
security/pkix/lib/pkixder.h
--- a/security/pkix/lib/pkixbuild.cpp
+++ b/security/pkix/lib/pkixbuild.cpp
@@ -251,18 +251,17 @@ BuildForward(TrustDomain& trustDomain,
        !CERT_LIST_END(n, candidates); n = CERT_LIST_NEXT(n)) {
     rv = BuildForwardInner(trustDomain, subject, time, endEntityOrCA,
                            requiredEKUIfPresent, requiredPolicy,
                            n->cert, subCACount, results);
     if (rv == Success) {
       // If we found a valid chain but deferred reporting an error with the
       // end-entity certificate, report it now.
       if (deferredEndEntityError != 0) {
-        PR_SetError(deferredEndEntityError, 0);
-        return FatalError;
+        return Fail(FatalError, deferredEndEntityError);
       }
 
       SECStatus srv = trustDomain.CheckRevocation(endEntityOrCA,
                                                   subject.GetNSSCert(),
                                                   n->cert, time,
                                                   stapledOCSPResponse);
       if (srv != SECSuccess) {
         return MapSECStatus(SECFailure);
@@ -274,18 +273,17 @@ BuildForward(TrustDomain& trustDomain,
     if (rv != RecoverableError) {
       return rv;
     }
 
     PRErrorCode currentError = PR_GetError();
     switch (currentError) {
       case 0:
         PR_NOT_REACHED("Error code not set!");
-        PR_SetError(PR_INVALID_STATE_ERROR, 0);
-        return FatalError;
+        return Fail(FatalError, PR_INVALID_STATE_ERROR);
       case SEC_ERROR_UNTRUSTED_CERT:
         currentError = SEC_ERROR_UNTRUSTED_ISSUER;
         break;
       default:
         break;
     }
     if (errorToReturn == 0) {
       errorToReturn = currentError;
--- a/security/pkix/lib/pkixcheck.cpp
+++ b/security/pkix/lib/pkixcheck.cpp
@@ -117,31 +117,29 @@ CheckCertificatePolicies(BackCert& cert,
     PR_SetError(SEC_ERROR_INVALID_ARGS, 0);
     return FatalError;
   }
 
   // Bug 989051. Until we handle inhibitAnyPolicy we will fail close when
   // inhibitAnyPolicy extension is present and we need to evaluate certificate
   // policies.
   if (cert.encodedInhibitAnyPolicy) {
-    PR_SetError(SEC_ERROR_POLICY_VALIDATION_FAILED, 0);
-    return RecoverableError;
+    return Fail(RecoverableError, SEC_ERROR_POLICY_VALIDATION_FAILED);
   }
 
   // The root CA certificate may omit the policies that it has been
   // trusted for, so we cannot require the policies to be present in those
   // certificates. Instead, the determination of which roots are trusted for
   // which policies is made by the TrustDomain's GetCertTrust method.
   if (isTrustAnchor && endEntityOrCA == MustBeCA) {
     return Success;
   }
 
   if (!cert.encodedCertificatePolicies) {
-    PR_SetError(SEC_ERROR_POLICY_VALIDATION_FAILED, 0);
-    return RecoverableError;
+    return Fail(RecoverableError, SEC_ERROR_POLICY_VALIDATION_FAILED);
   }
 
   ScopedPtr<CERTCertificatePolicies, CERT_DestroyCertificatePoliciesExtension>
     policies(CERT_DecodeCertificatePoliciesExtension(
                 cert.encodedCertificatePolicies));
   if (!policies) {
     return MapSECStatus(SECFailure);
   }
@@ -153,18 +151,17 @@ CheckCertificatePolicies(BackCert& cert,
     }
     // Intermediate certs are allowed to have the anyPolicy OID
     if (endEntityOrCA == MustBeCA &&
         (*policyInfos)->oid == SEC_OID_X509_ANY_POLICY) {
       return Success;
     }
   }
 
-  PR_SetError(SEC_ERROR_POLICY_VALIDATION_FAILED, 0);
-  return RecoverableError;
+  return Fail(RecoverableError, SEC_ERROR_POLICY_VALIDATION_FAILED);
 }
 
 //  BasicConstraints ::= SEQUENCE {
 //          cA                      BOOLEAN DEFAULT FALSE,
 //          pathLenConstraint       INTEGER (0..MAX) OPTIONAL }
 der::Result
 DecodeBasicConstraints(const SECItem* encodedBasicConstraints,
                        CERTBasicConstraints& basicConstraints)
@@ -349,18 +346,17 @@ CheckNameConstraints(BackCert& cert)
     PORT_Assert(names);
     CERTGeneralName* currentName = const_cast<CERTGeneralName*>(names);
     do {
       if (CERT_CheckNameSpace(arena, constraints, currentName) != SECSuccess) {
         // XXX: It seems like CERT_CheckNameSpace doesn't always call
         // PR_SetError when it fails. We set the error code here, though this
         // may be papering over some fatal errors. NSS's
         // cert_VerifyCertChainOld does something similar.
-        PR_SetError(SEC_ERROR_CERT_NOT_IN_NAME_SPACE, 0);
-        return RecoverableError;
+        return Fail(RecoverableError, SEC_ERROR_CERT_NOT_IN_NAME_SPACE);
       }
       currentName = CERT_GetNextGeneralName(currentName);
     } while (currentName != names);
   }
 
   return Success;
 }
 
@@ -410,18 +406,17 @@ CheckExtendedKeyUsage(EndEntityOrCA endE
       if (oidTag == SEC_OID_OCSP_RESPONDER) {
         foundOCSPSigning = true;
       }
     }
 
     // If the EKU extension was included, then the required EKU must be in the
     // list.
     if (!found) {
-      PR_SetError(SEC_ERROR_INADEQUATE_CERT_TYPE, 0);
-      return RecoverableError;
+      return Fail(RecoverableError, SEC_ERROR_INADEQUATE_CERT_TYPE);
     }
   }
 
   // pkixocsp.cpp depends on the following additional checks.
 
   if (endEntityOrCA == MustBeEndEntity) {
     // When validating anything other than an delegated OCSP signing cert,
     // reject any cert that also claims to be an OCSP responder, because such
@@ -474,18 +469,17 @@ CheckIssuerIndependentProperties(TrustDo
   rv = MapSECStatus(trustDomain.GetCertTrust(endEntityOrCA,
                                              requiredPolicy,
                                              cert.GetNSSCert(),
                                              &trustLevel));
   if (rv != Success) {
     return rv;
   }
   if (trustLevel == TrustDomain::ActivelyDistrusted) {
-    PORT_SetError(SEC_ERROR_UNTRUSTED_CERT);
-    return RecoverableError;
+    return Fail(RecoverableError, SEC_ERROR_UNTRUSTED_CERT);
   }
   if (trustLevel != TrustDomain::TrustAnchor &&
       trustLevel != TrustDomain::InheritsTrust) {
     // The TrustDomain returned a trust level that we weren't expecting.
     PORT_SetError(PR_INVALID_STATE_ERROR);
     return FatalError;
   }
   if (trustLevelOut) {
--- a/security/pkix/lib/pkixder.h
+++ b/security/pkix/lib/pkixder.h
@@ -371,18 +371,17 @@ Boolean(Input& input, /*out*/ bool& valu
   uint8_t intValue;
   if (input.Read(intValue) != Success) {
     return Failure;
   }
   switch (intValue) {
     case 0: value = false; return Success;
     case 0xFF: value = true; return Success;
     default:
-      PR_SetError(SEC_ERROR_BAD_DER, 0);
-      return Failure;
+      return Fail(SEC_ERROR_BAD_DER);
   }
 }
 
 // This is for any BOOLEAN DEFAULT FALSE.
 // (If it is present and false, this is a bad encoding.)
 // TODO(bug 989518): For compatibility reasons, in some places we allow
 // invalid encodings with the explicit default value.
 inline Result