bug 1404824 - fix error-handling case of plaintext-layer popping in nsNSSSocketInfo r=mayhemer
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 03 Oct 2017 14:29:31 -0700
changeset 384956 8ef54b47be709e5bc7861c78d4f825ddc64f0e19
parent 384955 645a700843fefbdf5c5dab0ec5f1fc7f13dcaf01
child 384957 c78938f9f8f82a234106c7af3ff8caf1c1881db5
push id95880
push userarchaeopteryx@coole-files.de
push dateSat, 07 Oct 2017 08:58:44 +0000
treeherdermozilla-inbound@156942799371 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmayhemer
bugs1404824
milestone58.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 1404824 - fix error-handling case of plaintext-layer popping in nsNSSSocketInfo r=mayhemer The PRFileDesc* returned by PR_PopIOLayer must be used rather than a preexisting pointer to the layer in question. MozReview-Commit-ID: 8PsCA5npaj6
security/manager/ssl/nsNSSIOLayer.cpp
--- a/security/manager/ssl/nsNSSIOLayer.cpp
+++ b/security/manager/ssl/nsNSSIOLayer.cpp
@@ -311,33 +311,35 @@ nsNSSSocketInfo::SetHandshakeCompleted()
 
     // If the handshake is completed for the first time from just 1 callback
     // that means that TLS session resumption must have been used.
     Telemetry::Accumulate(Telemetry::SSL_RESUMED_SESSION,
                           handshakeType == Resumption);
     Telemetry::Accumulate(Telemetry::SSL_HANDSHAKE_TYPE, handshakeType);
   }
 
-
-    // Remove the plain text layer as it is not needed anymore.
-    // The plain text layer is not always present - so its not a fatal error
-    // if it cannot be removed
+  // Remove the plaintext layer as it is not needed anymore.
+  // The plaintext layer is not always present - so it's not a fatal error if it
+  // cannot be removed.
+  // Note that PR_PopIOLayer may modify its stack, so a pointer returned by
+  // PR_GetIdentitiesLayer may not point to what we think it points to after
+  // calling PR_PopIOLayer. We must operate on the pointer returned by
+  // PR_PopIOLayer.
+  if (PR_GetIdentitiesLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity)) {
     PRFileDesc* poppedPlaintext =
-      PR_GetIdentitiesLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
-    if (poppedPlaintext) {
       PR_PopIOLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
-      poppedPlaintext->dtor(poppedPlaintext);
-    }
-
-    mHandshakeCompleted = true;
-
-    MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
-           ("[%p] nsNSSSocketInfo::SetHandshakeCompleted\n", (void*) mFd));
-
-    mIsFullHandshake = false; // reset for next handshake on this connection
+    poppedPlaintext->dtor(poppedPlaintext);
+  }
+
+  mHandshakeCompleted = true;
+
+  MOZ_LOG(gPIPNSSLog, LogLevel::Debug,
+          ("[%p] nsNSSSocketInfo::SetHandshakeCompleted\n", (void*) mFd));
+
+  mIsFullHandshake = false; // reset for next handshake on this connection
 }
 
 void
 nsNSSSocketInfo::SetNegotiatedNPN(const char* value, uint32_t length)
 {
   if (!value) {
     mNegotiatedNPN.Truncate();
   } else {
@@ -985,22 +987,25 @@ PRStatus
 nsNSSSocketInfo::CloseSocketAndDestroy(
     const nsNSSShutDownPreventionLock& /*proofOfLock*/)
 {
   PRFileDesc* popped = PR_PopIOLayer(mFd, PR_TOP_IO_LAYER);
   MOZ_ASSERT(popped &&
                popped->identity == nsSSLIOLayerHelpers::nsSSLIOLayerIdentity,
              "SSL Layer not on top of stack");
 
-  // The plain text layer is not always present - so its not a fatal error
-  // if it cannot be removed
-  PRFileDesc* poppedPlaintext =
-    PR_GetIdentitiesLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
-  if (poppedPlaintext) {
-    PR_PopIOLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
+  // The plaintext layer is not always present - so it's not a fatal error if it
+  // cannot be removed.
+  // Note that PR_PopIOLayer may modify its stack, so a pointer returned by
+  // PR_GetIdentitiesLayer may not point to what we think it points to after
+  // calling PR_PopIOLayer. We must operate on the pointer returned by
+  // PR_PopIOLayer.
+  if (PR_GetIdentitiesLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity)) {
+    PRFileDesc* poppedPlaintext =
+      PR_PopIOLayer(mFd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
     poppedPlaintext->dtor(poppedPlaintext);
   }
 
   PRStatus status = mFd->methods->close(mFd);
 
   // the nsNSSSocketInfo instance can out-live the connection, so we need some
   // indication that the connection has been closed. mFd == nullptr is that
   // indication. This is needed, for example, when the connection is closed
@@ -2812,13 +2817,17 @@ nsSSLIOLayerAddToSocket(int32_t family,
 
   return NS_OK;
  loser:
   NS_IF_RELEASE(infoObject);
   if (layer) {
     layer->dtor(layer);
   }
   if (plaintextLayer) {
-    PR_PopIOLayer(fd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
+    // Note that PR_*IOLayer operations may modify the stack of fds, so a
+    // previously-valid pointer may no longer point to what we think it points
+    // to after calling PR_PopIOLayer. We must operate on the pointer returned
+    // by PR_PopIOLayer.
+    plaintextLayer = PR_PopIOLayer(fd, nsSSLIOLayerHelpers::nsSSLPlaintextLayerIdentity);
     plaintextLayer->dtor(plaintextLayer);
   }
   return NS_ERROR_FAILURE;
 }