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>
Wed, 23 Jul 2014 13:14:19 -0700
changeset 208211 adc6758d943d
parent 208210 fd04691e6c71
child 208212 f3e354f0ec70
push id3771
push userryanvm@gmail.com
push date2014-07-31 17:19 +0000
treeherdermozilla-beta@adc6758d943d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersbriansmith, cviecco, lmandel
bugs1040889
milestone32.0
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
@@ -359,39 +359,56 @@ NSSCertDBTrustDomain::CheckRevocation(
 
   ScopedPLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
   if (!arena) {
     return SECFailure;
   }
 
   // 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(arena.get(), cert,
                                                     issuerCert));
     if (!request) {
       return SECFailure;
     }
 
     response = DoOCSPRequest(arena.get(), url.get(), 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(cert, issuerCert, 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(cert, issuerCert, 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;
     }
@@ -426,17 +443,17 @@ NSSCertDBTrustDomain::CheckRevocation(
                                                         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 "