# HG changeset patch
# User Robert Relyea <rrelyea@redhat.com>
# Date 1516007838 -3600
# Mon Jan 15 10:17:18 2018 +0100
# Node ID 33d9c969cd6548c335ce43fa8909b96ef323f670
# Parent db32ef3be38eb06a91babbcbb48285284d704dbd
Bug 1054373, Crash in PK11_DoesMechanism due to race condition, r=rsleevi
diff --git a/lib/dev/devslot.c b/lib/dev/devslot.c
--- a/lib/dev/devslot.c
+++ b/lib/dev/devslot.c
@@ -33,6 +33,8 @@ nssSlot_Destroy(
if (PR_ATOMIC_DECREMENT(&slot->base.refCount) == 0) {
PK11_FreeSlot(slot->pk11slot);
PZ_DestroyLock(slot->base.lock);
+ PZ_DestroyCondVar(slot->isPresentCondition);
+ PZ_DestroyLock(slot->isPresentLock);
return nssArena_Destroy(slot->base.arena);
}
}
@@ -117,35 +119,61 @@ nssSlot_IsTokenPresent(
nssSession *session;
CK_SLOT_INFO slotInfo;
void *epv;
+ PRBool isPresent = PR_FALSE;
+
/* permanent slots are always present unless they're disabled */
if (nssSlot_IsPermanent(slot)) {
return !PK11_IsDisabled(slot->pk11slot);
}
+
/* avoid repeated calls to check token status within set interval */
+ PZ_Lock(slot->isPresentLock);
if (within_token_delay_period(slot)) {
- return ((slot->ckFlags & CKF_TOKEN_PRESENT) != 0);
+ CK_FLAGS ckFlags = slot->ckFlags;
+ PZ_Unlock(slot->isPresentLock);
+ return ((ckFlags & CKF_TOKEN_PRESENT) != 0);
}
+ PZ_Unlock(slot->isPresentLock);
- /* First obtain the slot info */
+ /* First obtain the slot epv before we set up the condition
+ * variable, so we can just return if we couldn't get it. */
epv = slot->epv;
if (!epv) {
return PR_FALSE;
}
+
+ /* set up condition so only one thread is active in this part of the code at a time */
+ PZ_Lock(slot->isPresentLock);
+ while (slot->inIsPresent) {
+ PR_WaitCondVar(slot->isPresentCondition, 0);
+ }
+ /* if we were one of multiple threads here, the first thread will have
+ * given us the answer, no need to make more queries of the token. */
+ if (within_token_delay_period(slot)) {
+ CK_FLAGS ckFlags = slot->ckFlags;
+ PZ_Unlock(slot->isPresentLock);
+ return ((ckFlags & CKF_TOKEN_PRESENT) != 0);
+ }
+ /* this is the winning thread, block all others until we've determined
+ * if the token is present and that it needs initialization. */
+ slot->inIsPresent = PR_TRUE;
+ PZ_Unlock(slot->isPresentLock);
+
nssSlot_EnterMonitor(slot);
ckrv = CKAPI(epv)->C_GetSlotInfo(slot->slotID, &slotInfo);
nssSlot_ExitMonitor(slot);
if (ckrv != CKR_OK) {
slot->token->base.name[0] = 0; /* XXX */
- slot->lastTokenPing = PR_IntervalNow();
- return PR_FALSE;
+ isPresent = PR_FALSE;
+ goto done;
}
slot->ckFlags = slotInfo.flags;
/* check for the presence of the token */
if ((slot->ckFlags & CKF_TOKEN_PRESENT) == 0) {
if (!slot->token) {
/* token was never present */
- slot->lastTokenPing = PR_IntervalNow();
- return PR_FALSE;
+ isPresent = PR_FALSE;
+ goto done;
}
session = nssToken_GetDefaultSession(slot->token);
if (session) {
@@ -167,15 +195,15 @@ nssSlot_IsTokenPresent(
slot->token->base.name[0] = 0; /* XXX */
/* clear the token cache */
nssToken_Remove(slot->token);
- slot->lastTokenPing = PR_IntervalNow();
- return PR_FALSE;
+ isPresent = PR_FALSE;
+ goto done;
}
/* token is present, use the session info to determine if the card
* has been removed and reinserted.
*/
session = nssToken_GetDefaultSession(slot->token);
if (session) {
- PRBool isPresent = PR_FALSE;
+ PRBool tokenRemoved;
nssSession_EnterMonitor(session);
if (session->handle != CK_INVALID_SESSION) {
CK_SESSION_INFO sessionInfo;
@@ -187,12 +215,12 @@ nssSlot_IsTokenPresent(
session->handle = CK_INVALID_SESSION;
}
}
- isPresent = session->handle != CK_INVALID_SESSION;
+ tokenRemoved = (session->handle == CK_INVALID_SESSION);
nssSession_ExitMonitor(session);
/* token not removed, finished */
- if (isPresent) {
- slot->lastTokenPing = PR_IntervalNow();
- return PR_TRUE;
+ if (!tokenRemoved) {
+ isPresent = PR_TRUE;
+ goto done;
}
}
/* the token has been removed, and reinserted, or the slot contains
@@ -203,15 +231,27 @@ nssSlot_IsTokenPresent(
nssToken_Remove(slot->token);
/* token has been removed, need to refresh with new session */
nssrv = nssSlot_Refresh(slot);
+ isPresent = PR_TRUE;
if (nssrv != PR_SUCCESS) {
slot->token->base.name[0] = 0; /* XXX */
slot->ckFlags &= ~CKF_TOKEN_PRESENT;
- /* TODO: insert a barrier here to avoid reordering of the assingments */
- slot->lastTokenPing = PR_IntervalNow();
- return PR_FALSE;
+ isPresent = PR_FALSE;
}
+done:
+ /* Once we've set up the condition variable,
+ * Before returning, it's necessary to:
+ * 1) Set the lastTokenPing time so that any other threads waiting on this
+ * initialization and any future calls within the initialization window
+ * return the just-computed status.
+ * 2) Indicate we're complete, waking up all other threads that may still
+ * be waiting on initialization can progress.
+ */
+ PZ_Lock(slot->isPresentLock);
slot->lastTokenPing = PR_IntervalNow();
- return PR_TRUE;
+ slot->inIsPresent = PR_FALSE;
+ PR_NotifyAllCondVar(slot->isPresentCondition);
+ PZ_Unlock(slot->isPresentLock);
+ return isPresent;
}
NSS_IMPLEMENT void *
@@ -229,7 +269,7 @@ nssSlot_GetToken(
if (nssSlot_IsTokenPresent(slot)) {
/* Even if a token should be present, check `slot->token` too as it
- * might be gone already. This would happen mostly on shutdown. */
+ * might be gone already. This would happen mostly on shutdown. */
nssSlot_EnterMonitor(slot);
if (slot->token)
rvToken = nssToken_AddRef(slot->token);
diff --git a/lib/dev/devt.h b/lib/dev/devt.h
--- a/lib/dev/devt.h
+++ b/lib/dev/devt.h
@@ -81,6 +81,9 @@ struct NSSSlotStr {
PZLock *lock;
void *epv;
PK11SlotInfo *pk11slot;
+ PZLock *isPresentLock;
+ PRCondVar *isPresentCondition;
+ PRBool inIsPresent;
};
struct nssSessionStr {
diff --git a/lib/pk11wrap/dev3hack.c b/lib/pk11wrap/dev3hack.c
--- a/lib/pk11wrap/dev3hack.c
+++ b/lib/pk11wrap/dev3hack.c
@@ -120,6 +120,9 @@ nssSlot_CreateFromPK11SlotInfo(NSSTrustD
/* Grab the slot name from the PKCS#11 fixed-length buffer */
rvSlot->base.name = nssUTF8_Duplicate(nss3slot->slot_name, td->arena);
rvSlot->lock = (nss3slot->isThreadSafe) ? NULL : nss3slot->sessionLock;
+ rvSlot->isPresentLock = PZ_NewLock(nssiLockOther);
+ rvSlot->isPresentCondition = PR_NewCondVar(rvSlot->isPresentLock);
+ rvSlot->inIsPresent = PR_FALSE;
return rvSlot;
}