Bug 1040889 - Don't re-cache OCSP server failures if no fetch was attempted. r=briansmith, r=cviecco, a=lmandel
authorDavid Keeler <dkeeler@mozilla.com>
Fri, 25 Jul 2014 16:59:22 -0700
changeset 217324 6c3c6a9f30c120ce70cddd275a08b12ccda58c2d
parent 217323 82a8d3181fa97d3d1e0ef57795ae76196fe2e20b
child 217325 34fe3ed2fb644b06400acc3ee6e6c0add20189cb
push id515
push userraliiev@mozilla.com
push dateMon, 06 Oct 2014 12:51:51 +0000
treeherdermozilla-release@267c7a481bef [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith, cviecco, lmandel
bugs1040889
milestone33.0a2
Bug 1040889 - Don't re-cache OCSP server failures if no fetch was attempted. r=briansmith, r=cviecco, a=lmandel
security/certverifier/NSSCertDBTrustDomain.cpp
--- a/security/certverifier/NSSCertDBTrustDomain.cpp
+++ b/security/certverifier/NSSCertDBTrustDomain.cpp
@@ -488,39 +488,56 @@ NSSCertDBTrustDomain::CheckRevocation(En
     // assume it is good. Note that this is the confusing, but intended,
     // interpretation of "strict" revocation checking in the face of a
     // certificate that lacks an OCSP responder URI.
     return SECSuccess;
   }
 
   // Only request a response if we didn't have a cached indication of failure
   // (don't keep requesting responses from a failing server).
-  const SECItem* response = nullptr;
+  const SECItem* response;
+  bool attemptedRequest;
+  // Initialize error here to a value we check that it isn't later, because
+  // the compiler on windows isn't smart enough to figure out that it's
+  // always initialized before it gets used.
+  PRErrorCode error = 0;
   if (cachedResponseErrorCode == 0 ||
       cachedResponseErrorCode == SEC_ERROR_OCSP_UNKNOWN_CERT ||
       cachedResponseErrorCode == SEC_ERROR_OCSP_OLD_RESPONSE) {
     const SECItem* request(CreateEncodedOCSPRequest(*this, arena.get(),
                                                     certID));
     if (!request) {
       return SECFailure;
     }
 
     response = DoOCSPRequest(arena.get(), url, request,
                              OCSPFetchingTypeToTimeoutTime(mOCSPFetching),
                              mOCSPGetConfig == CertVerifier::ocsp_get_enabled);
+    if (!response) {
+      error = PR_GetError();
+    }
+    attemptedRequest = true;
+  } else {
+    error = cachedResponseErrorCode;
+    response = nullptr;
+    attemptedRequest = false;
   }
 
+  // If we don't have a response, either something went wrong when fetching it
+  // or we didn't attempt to fetch a response because of a failing responder.
   if (!response) {
-    PRErrorCode error = PR_GetError();
-    if (error == 0) {
-      error = cachedResponseErrorCode;
-    }
-    PRTime timeout = time + ServerFailureDelay;
-    if (mOCSPCache.Put(certID, error, time, timeout) != SECSuccess) {
-      return SECFailure;
+    MOZ_ASSERT(error != 0);
+    // If we haven't actually attempted to fetch a response, we have nothing
+    // new to tell the cache. Otherwise, we do.
+    if (attemptedRequest) {
+      PRTime timeout = time + ServerFailureDelay;
+      SECStatus rv = mOCSPCache.Put(certID, error, time, timeout);
+      if (rv != SECSuccess) {
+        return SECFailure;
+      }
     }
     PR_SetError(error, 0);
     if (mOCSPFetching != FetchOCSPForDVSoftFail) {
       PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
              ("NSSCertDBTrustDomain: returning SECFailure after "
               "OCSP request failure"));
       return SECFailure;
     }
@@ -555,17 +572,17 @@ NSSCertDBTrustDomain::CheckRevocation(En
                                                         ResponseIsFromNetwork,
                                                         expired);
   if (rv == SECSuccess || mOCSPFetching != FetchOCSPForDVSoftFail) {
     PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
       ("NSSCertDBTrustDomain: returning after VerifyEncodedOCSPResponse"));
     return rv;
   }
 
-  PRErrorCode error = PR_GetError();
+  error = PR_GetError();
   if (error == SEC_ERROR_OCSP_UNKNOWN_CERT ||
       error == SEC_ERROR_REVOKED_CERTIFICATE) {
     return rv;
   }
 
   if (stapledOCSPResponseErrorCode != 0) {
     PR_LOG(gCertVerifierLog, PR_LOG_DEBUG,
            ("NSSCertDBTrustDomain: returning SECFailure from expired stapled "