bug 1404824 - fix error-handling case of plaintext-layer popping in nsNSSSocketInfo r?mayhemer draft
authorDavid Keeler <dkeeler@mozilla.com>
Tue, 03 Oct 2017 14:29:31 -0700
changeset 676075 73aefbaed5d15949c61ec1241e372ccb33ebf179
parent 675689 19b32a138d08f73961df878a29de6f0aad441683
child 676076 f7d56b907ee320ca1af259e135bd2a2651e64601
push id83391
push userbmo:dkeeler@mozilla.com
push dateFri, 06 Oct 2017 16:49:13 +0000
reviewersmayhemer
bugs1404824
milestone58.0a1
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;
 }