Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Bug 1682863 - Revert nssSlot_IsTokenPresent to 3.58 after ongoing Fx …
…hangs with slow PKCS11 devices. r=bbeurdouche

This patch reverts the `nssSlot_IsTokenPresent` changes made in bug 1663661
and bug 1679290, restoring the version used in NSS 3.58 and earlier. It's not an
actual `hg backout` because the comment in lib/dev/devt.h is worth keeping.
While removing the nested locking did resolve the hang for some (most?) third-party
modules, problems remain with some slower tokens after an even further relaxation
of the locking, which defeats the purpose of addressing the races in the first place.

The crash addressed by these patches was caused by the Intermediate Preloading
Healer in Firefox, which has been disabled. We clearly have insufficient test
coverage for third-party modules, and now that osclientcerts is enabled in Fx
Nightly, any problems caused by these and similar changes is unlikely to be
reported until Fx Beta, well after NSS RTM. I think the best option at this
point is to simply revert NSS.

Differential Revision: https://phabricator.services.mozilla.com/D100344

--HG--
extra : moz-landing-system : lando
  • Loading branch information
Kevin Jacobs committed Dec 22, 2020
1 parent 0dfd3aa commit fac0bad
Showing 1 changed file with 6 additions and 30 deletions.
36 changes: 6 additions & 30 deletions lib/dev/devslot.c
Expand Up @@ -171,24 +171,22 @@ nssSlot_IsTokenPresent(

nssSlot_EnterMonitor(slot);
ckrv = CKAPI(epv)->C_GetSlotInfo(slot->slotID, &slotInfo);
nssSlot_ExitMonitor(slot);
if (ckrv != CKR_OK) {
if (slot->token) {
slot->token->base.name[0] = 0; /* XXX */
}
slot->token->base.name[0] = 0; /* XXX */
isPresent = PR_FALSE;
goto done; /* slot lock held */
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 */
isPresent = PR_FALSE;
goto done; /* slot lock held */
goto done;
}
session = nssToken_GetDefaultSession(slot->token);
if (session) {
nssSlot_ExitMonitor(slot);
nssSession_EnterMonitor(session);
/* token is not present */
if (session->handle != CK_INVALID_HANDLE) {
Expand All @@ -198,12 +196,6 @@ nssSlot_IsTokenPresent(
session->handle = CK_INVALID_HANDLE;
}
nssSession_ExitMonitor(session);
nssSlot_EnterMonitor(slot);
if (!slot->token) {
/* Check token presence after re-acquiring lock */
isPresent = PR_FALSE;
goto done; /* slot lock held */
}
}
if (slot->token->base.name[0] != 0) {
/* notify the high-level cache that the token is removed */
Expand All @@ -214,23 +206,14 @@ nssSlot_IsTokenPresent(
/* clear the token cache */
nssToken_Remove(slot->token);
isPresent = PR_FALSE;
goto done; /* slot lock held */
}
if (!slot->token) {
/* This should not occur, based on the fact that the
* below calls will dereference NULL otherwise. */
PORT_Assert(0);
isPresent = PR_FALSE;
goto done; /* slot lock held */
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 tokenRemoved;
nssSlot_ExitMonitor(slot);
nssSession_EnterMonitor(session);
if (session->handle != CK_INVALID_HANDLE) {
CK_SESSION_INFO sessionInfo;
Expand All @@ -244,16 +227,10 @@ nssSlot_IsTokenPresent(
}
tokenRemoved = (session->handle == CK_INVALID_HANDLE);
nssSession_ExitMonitor(session);
nssSlot_EnterMonitor(slot);
/* token not removed, finished */
if (!tokenRemoved) {
isPresent = PR_TRUE;
goto done; /* slot lock held */
}
if (!slot->token) {
/* Check token presence after re-acquiring lock */
isPresent = PR_FALSE;
goto done; /* slot lock held */
goto done;
}
}
/* the token has been removed, and reinserted, or the slot contains
Expand All @@ -271,7 +248,6 @@ nssSlot_IsTokenPresent(
isPresent = PR_FALSE;
}
done:
nssSlot_ExitMonitor(slot);
/* Once we've set up the condition variable,
* Before returning, it's necessary to:
* 1) Set the lastTokenPingTime so that any other threads waiting on this
Expand Down

0 comments on commit fac0bad

Please sign in to comment.