# HG changeset patch # User Robert Relyea # 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; }