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
--- 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;
}