Closed Bug 588269 Opened 14 years ago Closed 12 years ago

SECMOD_CloseUserDB fails

Categories

(NSS :: Libraries, defect, P2)

Product:
NSS
Component:
Libraries
Type:
defect

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mattm, Assigned: ryan.sleevi)

References

Details

Attachments

(2 files, 5 obsolete files)

test code
14.63 KB, text/plain
Details
Patch by Ryan Sleevi (as checked in)
10.20 KB, patch
Details | Diff | Splinter Review
Attached file test code —
I'm working on some tests for Chromium that use SECMOD_OpenUserDB and SECMOD_CloseUserDB around each test to keep the tests self contained. I see bug 506140, which sounds similar, but I'm seeing different behavior so filing separately. When using NSS cvs trunk, SECMOD_CloseUserDB fails with PORT_GetError of -8018. I used gdb and stepped through, the problems looks to be sftk_CreateNewSlot checks slot->slotID, and fails if it is not NETSCAPE_SLOT_ID(2) or FIPS_SLOT_ID(3). For the slot returned by SECMOD_OpenUserDB, the slotID will be 4 (or greater), so this check fails. Here's the backtrace for where sftk_CreateNewSlot is getting called in the close path: #0 sftk_CreateNewSlot (slot=0x8082e78, class=3461563222, object=0x80ca470) at pkcs11.c:3709 #1 0xf7bc7a0e in NSC_CreateObject (hSession=33554433, pTemplate=0xffffcc50, ulCount=2, phObject=0xffffcc70) at pkcs11.c:3826 #2 0xf7e571b9 in PK11_CreateNewObject (slot=0x80b51b0, session=33554433, theTemplate=0xffffcc50, count=2, token=0, objectID=0xffffcc70) at pk11obj.c:412 #3 0xf7e6cb53 in secmod_UserDBOp (slot=0x80b51b0, objClass=3461563222, sendSpec=0x80ca360 "tokens=[0x4=<>]") at pk11util.c:1298 #4 0xf7e6ceba in SECMOD_CloseUserDB (slot=0x80b51b0) at pk11util.c:1503 #5 0x08048ed4 in TestUserDB (userdbdir=0xffffce36 "/tmp/userdbtesthaVTxp/0") at userdbtest.c:259 #6 0x08049129 in main () at userdbtest.c:296 (If I ignore the return value of SECMOD_CloseUserDB as some code seems to do, then subsequent tests fail because the certificates in the userdb are still present. In the attached test program, the second iteration would fail with "temp certificate is already permanent!".) I've attached a test program. When run with CVS NSS I get this output: opening main db: sql:/tmp/userdbtestMnyDAM/main Iteration 0 opening userdb: configDir='sql:/tmp/userdbtestMnyDAM/0' tokenDescription='/tmp/userdbtestMnyDAM/0' token name:/tmp/userdbtestMnyDAM/0 slot name:NSS Application Slot 00000004 SECMOD_CloseUserDB failed: -8018 I get similar output with NSS 3.12.[467] except with error -8192 instead. Also, perhaps of interest, with NSS 3.12.3.1 SECMOD_CloseUserDB does not fail, but instead after 9 iterations the next call to SECMOD_OpenUserDB will return the same slot as the first call, and I get weird failures: Iteration 8 opening userdb: configDir='sql:/tmp/userdbtest0qrnb2/8' tokenDescription='/tmp/userdbtest0qrnb2/8' token name:/tmp/userdbtest0qrnb2/0 slot name:NSS Application Slot 00000004 CERT_ChangeCertTrust failed: -8037
Assignee: nobody → rrelyea
Attachment #466894 - Attachment mime type: text/x-csrc → text/plain
Priority: -- → P2
Target Milestone: --- → 3.12.9
So the issue seems that sftk_CreateNewSlot is hardcoded to only work with the NETSCAPE_SLOT_ID/FIPS_SLOT_ID, as you noted. This is shown at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11.c&rev=1.179&mark=3907,3921,3926,3931#3907 When adding a module, via SECMOD_OpenUserDB, NSS first grabs the internal module and then passes that to SECMOD_OpenNewSlot() - http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11util.c&rev=1.63&mark=1449,1459,1465#1449 . SECMOD_OpenNewSlot attempts to find the highest unused slot ID, then grabs unconditionally the /first/ slot, creates the slot spec, and passes that to secmod_UserDBOp - http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11util.c&rev=1.63&mark=1351,1359,1369,1389#1351 When calling SECMOD_CloseUserDB, however, simply calls the operation on the slot the caller provided - which will NEVER equal the NETSCAPE or FIPS slot IDs - http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11util.c&rev=1.63&mark=1476,1487#1476 What is really desired is that the logic from http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11.c&rev=1.179&mark=3907,3938-3939,3958-3959,3967-3973#3907 should be able to execute before any of the slot ID checks. I'd propose the following change to softoken, and will work on a patch if this sounds correct: sftk_CreateNewSlot should do the following: - Ensure that the class is CKO_NETSCAPE_NEWSLOT or CKO_NETSCAPE_DELSLOT - Ensure that the CKA_NETSCAPE_MODULE_SPEC is present and properly parses - to a single parameter, that contains a single slot ID - If class is CKO_NETSCAPE_DELSLOT * Find the existing slot with that slot id. The operation will FAIL ( CKR_SLOT_ID_INVALID ) if the slot ID is invalid * Call SFTK_ShutdownSlot and shut down the existing slot * Return - If class is CKO_NETSCAPE_NEWSLOT * Verify the slot ID is valid for the MIN/MAX slot IDs of the 'calling' slot (that is, the first slot in the internal module) * Find the existing slot with that slot ID. If it exists, shut it down. * Call SFTK_SlotReInit if the slot previously existed, SFTK_SlotInit if it's a brand new slot This means there is NO CHANGE to the behaviour of CKO_NETSCAPE_NEWSLOT, but should fix the CKO_NETSCAPE_DELSLOT Does this sound like the right behaviour?
Ryan: thank you for tracking this down. I agree with your analysis. (In reply to Ryan Sleevi from comment #1) > > I'd propose the following change to softoken, and will work on a patch if > this sounds correct: > sftk_CreateNewSlot should do the following: > - Ensure that the class is CKO_NETSCAPE_NEWSLOT or CKO_NETSCAPE_DELSLOT > - Ensure that the CKA_NETSCAPE_MODULE_SPEC is present and properly parses > - to a single parameter, that contains a single slot ID It seems that we already ensure that CKA_NETSCAPE_MODULE_SPEC parses to a single parameter: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11.c&rev=1.179&mark=3907,3938-3939,3944-3948#3907 Are you suggesting that we should also ensure that the other members of paramStrings, such as configdir and updatedir, are all NULL or PR_FALSE? > - If class is CKO_NETSCAPE_DELSLOT > * Find the existing slot with that slot id. The operation will FAIL ( > CKR_SLOT_ID_INVALID ) if the slot ID is invalid Should we also check that the found slot is the same as the 'slot' input argument? Alternatively, we don't need to find the existing slot with that slot id because the slot is already passed to the function. We just ensure that (slotID = paramStrings.tokens[0].slotID) == slot->slotID.
Assignee: rrelyea → ryan.sleevi
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
Target Milestone: 3.12.9 → 3.14
(In reply to Wan-Teh Chang from comment #2) > Ryan: thank you for tracking this down. I agree with your analysis. > > (In reply to Ryan Sleevi from comment #1) > > > > I'd propose the following change to softoken, and will work on a patch if > > this sounds correct: > > sftk_CreateNewSlot should do the following: > > - Ensure that the class is CKO_NETSCAPE_NEWSLOT or CKO_NETSCAPE_DELSLOT > > - Ensure that the CKA_NETSCAPE_MODULE_SPEC is present and properly parses > > - to a single parameter, that contains a single slot ID > > It seems that we already ensure that CKA_NETSCAPE_MODULE_SPEC parses to a > single parameter: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/ > softoken/pkcs11.c&rev=1.179&mark=3907,3938-3939,3944-3948#3907 > > Are you suggesting that we should also ensure that the other members of > paramStrings, such as configdir and updatedir, are all NULL or PR_FALSE? I was merely re-stating the existing requirements, showing when they would be evaluated in terms of processing. > > > - If class is CKO_NETSCAPE_DELSLOT > > * Find the existing slot with that slot id. The operation will FAIL ( > > CKR_SLOT_ID_INVALID ) if the slot ID is invalid > > Should we also check that the found slot is the same as the 'slot' input > argument? > > Alternatively, we don't need to find the existing slot with that slot id > because the slot is already passed to the function. We just ensure that > (slotID = paramStrings.tokens[0].slotID) == slot->slotID. I think this check makes sense. It's a new check/change in behaviour, but it matches with how SECMOD_CloseUserDB is documented as expected to behave, so this seems correct. Given that it's presently broken, I can't imagine this additional requirement will break any existing code.
Attached patch First stab (obsolete) — — Splinter Review
I've attached a possible fix https://bugzilla.mozilla.org/attachment.cgi?id=641705 , but it's currently incomplete. The above code works for the first open and close case, the second attempt to open succeeds, but the attempt to close the second handle fails within NSC_CreateObject: (gdb) bt #0 NSC_CreateObject (hSession=33554433, pTemplate=0xffffcffc, ulCount=2, phObject=0xffffd01c) at pkcs11.c:4020 #1 0xf7e51c09 in PK11_CreateNewObject (slot=0x808e3e0, session=33554433, theTemplate=0xffffcffc, count=2, token=0, objectID=0xffffd01c) at pk11obj.c:378 #2 0xf7e655f7 in secmod_UserDBOp (slot=0x808e3e0, objClass=3461563222, sendSpec=0x8091bf8 "tokens=[0x4=<>]") at pk11util.c:1282 #3 0xf7e65929 in SECMOD_CloseUserDB (slot=0x808e3e0) at pk11util.c:1487 #4 0x0804933a in TestOpenCloseUserDB (progName=0xffffd342 "secmodtest", configDir=0x804f4a0 "sql:/tmp/test2", tokenName=0x804b239 "Test Slot 2") at secmodtest.c:54 #5 0x0804951f in main (argc=5, argv=0xffffd184) at secmodtest.c:111 NSC_CreateObject fails because sftk_SessionFromHandle ends up failing to find a/the session. Specifically, http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/softoken/pkcs11.c&rev=1.179&mark=3995-3997,4038-4043#3995 The contents of *slot are: $4 = { slotID = 4, slotLock = 0x805d7c8, sessionLock = 0x805d828, numSessionLocks = 512, sessionLockMask = 511, objectLock = 0x806a030, pwCheckLock = 0x806a090, present = 1, hasTokens = 0, isLoggedIn = 0, ssoLoggedIn = 0, needLogin = 1, DB_loaded = 0, readOnly = 0, optimizeSpace = 0, certDB = 0x80918b0, keyDB = 0x80918f0, minimumPinLen = 0, sessionIDCount = 2, sessionIDConflict = 0, sessionCount = 0, rwSessionCount = 0, sessionObjectHandleCount = 1, index = 2, tokObjHashTable = 0x805d658, sessObjHashTable = 0x806b0f8, sessObjHashSize = 1024, head = 0x806a0f0, sessHashSize = 1024, tokDescription = "Test Slot 2", ' ' <repeats 21 times>, updateTokDescription = ' ' <repeats 32 times>, slotDescription = "NSS Application Slot 00000004", ' ' <repeats 35 times> } In the first open/close pair, at open, sessionIDCount = 0, sessionCount = 0. At close, sessionIDCount = 2 at close, sessionCount = 1 So with the above code, what I'm not seeing is that when we re-open the slot, a new session to the slot is *not* being granted (sessionCount == 0, sessionIDCount is not incrementing, and NSC_OpenSession is never called. It's possible my test case is wrong - which would be great, and I'd appreciate an extra set of eyes on it. Otherwise, it would suggest that there is some bug deep in the session tracking code that is causing an object to be re-used.
Bob, could you take a look at this? The issue turned out to be that when a token ID was re-used between two (OpenUserDB,CloseUserDB) call pairs, the token session ID was not getting reset, even though the first CloseUserDB invalidated all the sessions. As a result, attempting to use the DB after the second OpenUserDB would fail, because the slot's session ID would not exist in the newly-opened softoken slot. I think the alternative to this approach would be to alter http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11slot.c&rev=1.113&mark=1393,1409-1410,1429,1435,1445-1448#1387 Instead of returning nssToken_IsPresent() directly when there is a slot->nssTrust, if nssToken_IsPresent() returned false, it would continue through the shutdown-and-reinit sequence below. I feel like this patch does the same though (PK11_InitToken to start up, setting session = CK_INVALID_SESSION on shutdown), and is easier to understand. WDYT?
Attachment #641705 - Attachment is obsolete: true
Attachment #642160 - Flags: review?(rrelyea)
Attachment #642160 - Flags: feedback?(wtc)
Comment on attachment 642160 [details] [diff] [review] Fix & Test to ensure correct behaviour Review of attachment 642160 [details] [diff] [review]: ----------------------------------------------------------------- This seems correct to me, although I don't fully understand the change that allows us to reuse a previously closed slot. The current fix is to call PK11_InitToken in the pk11wrap layer. It seems that ideally the fix should be contained in the softoken, perhaps by changing SFTK_ShutdownSlot or SFTK_SlotReInit to do more cleanup when we close or reopen a user DB. Is that possible? I like the test program you added. All of my comments below are about nits. ::: mozilla/security/nss/cmd/tests/secmodtest.c @@ +22,5 @@ > +#include "plgetopt.h" > + > +void Usage(char *progName) > +{ > + fprintf(stderr, "Usage: %s -d dbDir -u userDbDir\n", progName); The "-d dbDir" option is not being used. Remove it? @@ +26,5 @@ > + fprintf(stderr, "Usage: %s -d dbDir -u userDbDir\n", progName); > + exit(1); > +} > + > + Nit: delete this blank line. @@ +27,5 @@ > + exit(1); > +} > + > + > +SECStatus TestOpenCloseUserDB(char* progName, char *configDir, char *tokenName) Nit: put '*' next to the variable/argument. Please search for "* " in this file to fix all the occurrences. @@ +56,5 @@ > + SECU_PrintError(progName, "couldn't close user database"); > + goto shutdown; > + } > + > + PK11_FreeSlot(userDbSlot); Nit: PK11_FreeSlot probably should also be called when SECMOD_CloseUserDB fails. @@ +58,5 @@ > + } > + > + PK11_FreeSlot(userDbSlot); > + > +shutdown: Nit: this kind of label is named "loser" in NSS by convention. Similarly for the 'shutdown' label in the 'main' function. @@ +77,5 @@ > + progName = strrchr(argv[0], '\\'); > + } > + progName = progName ? progName+1 : argv[0]; > + > + optstate = PL_CreateOptState(argc, argv, "d:u:"); Remove "d:" and the dbDir variable? dbDir is not being used. @@ +92,5 @@ > + if (optstatus == PL_OPT_BAD || userDbDir == NULL) { > + Usage(progName); > + } > + > + PR_Init(PR_SYSTEM_THREAD, PR_PRIORITY_NORMAL, 1); Remove this PR_Init call. PR_Init is deprecated and should not be used in new code. ::: mozilla/security/nss/lib/pk11wrap/pk11util.c @@ +1294,4 @@ > * return true if the selected slot ID is not present or doesn't exist > */ > static PRBool > +secmod_SlotIsEmpty(SECMODModule *mod, CK_SLOT_ID slotID, PRBool* slot_exists) Nit: slot_exists => slotExists Please update the comment for this function to describe the slotExists output argument. *slotExists should be set to PR_FALSE explicitly if the slot doesn't exist. I suggest we do this before the "return PR_TRUE" statement: /* it doesn't exist or isn't present, it's available */ + *slotExists = (slot != NULL); return PR_TRUE; @@ -1304,2 @@ > return PR_FALSE; > - } Keep the curly braces. @@ +1312,4 @@ > * Find an unused slot id in module. > */ > static CK_SLOT_ID > +secmod_FindFreeSlot(SECMODModule *mod, PRBool* slot_exists) Nit: slot_exists => slotExists Please update the comment for this function to describe the slotExists output argument. @@ +1351,4 @@ > SECMOD_OpenNewSlot(SECMODModule *mod, const char *moduleSpec) > { > CK_SLOT_ID slotID = 0; > + PRBool existing_slot = PR_FALSE; Nit: existing_slot => existingSlot or slotExists ::: mozilla/security/nss/lib/softoken/pkcs11.c @@ +3904,5 @@ > + * the slot the request was passed to. > + * When removing a slot, the slot that is passed in is the slot to be > + * removed. > + * object is the creation object that specifies the module spec for the slot > + * to add or remove. When removing a slot, 'object' @@ +3910,5 @@ > static CK_RV sftk_CreateNewSlot(SFTKSlot *slot, CK_OBJECT_CLASS class, > SFTKObject *object) > { > + PRBool isValidUserSlot = PR_FALSE; > + PRBool isValidFIPSSlot = PR_FALSE; Should this variable be named isValidFIPSUserSlot? @@ +3916,2 @@ > PRBool isFIPS = PR_FALSE; > unsigned long moduleIndex; We may need to initialize moduleIndex to some value to silence the "variable may be used unintialized" compiler warning. In the original code, it is clearer that moduleIndex is always initialized. @@ +3926,3 @@ > return CKR_ATTRIBUTE_VALUE_INVALID; > } > + if (class == CKO_NETSCAPE_NEWSLOT && slot->slotID == FIPS_SLOT_ID) { It seems that the "class == CKO_NETSCAPE_NEWSLOT" test isn't necessary. If 'class' is CKO_NETSCAPE_DELSLOT, then slot->slotID cannot possibly be FIPS_SLOT_ID. BUT, if I understand the code correctly, the value of 'isFIPS' shouldn't matter in this function because the CKA_NETSCAPE_MODULE_SPEC attribute should specify "tokens=...", in which case sftk_parseParameters ignores its isFIPS argument. @@ +3955,5 @@ > + if (class == CKO_NETSCAPE_DELSLOT) { > + if (slot->slotID == slotID && (isValidUserSlot || isValidFIPSSlot)) { > + isValidSlot = PR_TRUE; > + } > + } else if (class == CKO_NETSCAPE_NEWSLOT) { Nit: this can be simply "else" because we already ensured 'class' is either CKO_NETSCAPE_DELSLOT or CKO_NETSCAPE_NEWSLOT. @@ +3984,4 @@ > /* if we were just planning on deleting the slot, then do so now */ > if (class == CKO_NETSCAPE_DELSLOT) { > /* sort of a unconventional use of this error code, be we are > + * overusing CKR_ATTRIBUTE_VALUE_INVALID, and it does apply */ There seems to be some whitespace change on this line (spaces <-> TABs?).
Attachment #642160 - Flags: feedback?(wtc) → feedback+
Comment on attachment 642160 [details] [diff] [review] Fix & Test to ensure correct behaviour Review of attachment 642160 [details] [diff] [review]: ----------------------------------------------------------------- ::: mozilla/security/nss/lib/softoken/pkcs11.c @@ +3904,5 @@ > + * the slot the request was passed to. > + * When removing a slot, the slot that is passed in is the slot to be > + * removed. > + * object is the creation object that specifies the module spec for the slot > + * to add or remove. I'm not sure I grokked what you were asking for, but I've tried rewording the comment. Please let me know if this addresses your concerns @@ +3926,3 @@ > return CKR_ATTRIBUTE_VALUE_INVALID; > } > + if (class == CKO_NETSCAPE_NEWSLOT && slot->slotID == FIPS_SLOT_ID) { I think you misread - your comment describes two different conditions. If class == CKO_NETSCAPE_NEWSLOT, then slot->slotID absolutely can be FIPS_SLOT_ID. If class == CKO_NETSCAPE_DELSLOT, then slot->slotID *should* not be FIPS_SLOT_ID. The difference being is that the API should and is guarding against invalid inputs. This check (line 3928) happens before we've done the range checking of slot->slotID (line 3956), so I do think this check adds value. More generally speaking, the interpretation of tokens= in sftk_parseParameters is contingent upon the value of isFIPS. While functionally it should be the same, the subtlety warranted me trying to preserve the original API behaviour for the NEW_SLOT case. @@ +3984,4 @@ > /* if we were just planning on deleting the slot, then do so now */ > if (class == CKO_NETSCAPE_DELSLOT) { > /* sort of a unconventional use of this error code, be we are > + * overusing CKR_ATTRIBUTE_VALUE_INVALID, and it does apply */ Yes. This comment had inconsistent spacing (it used tab for the first line, 8 space for the second). It seems like all code at a particular indent level should match the same spacing/tab depth. Is that wrong? Is there an NSS style guide anywhere?
Attached patch Patch 3: Address wtc's comments (obsolete) — — Splinter Review
Updated based on wtc's feedback.
Attachment #642160 - Attachment is obsolete: true
Attachment #642160 - Flags: review?(rrelyea)
Attachment #643486 - Flags: review?(rrelyea)
(In reply to Wan-Teh Chang from comment #7) > Comment on attachment 642160 [details] [diff] [review] > Fix & Test to ensure correct behaviour > > Review of attachment 642160 [details] [diff] [review]: > ----------------------------------------------------------------- > > This seems correct to me, although I don't fully understand > the change that allows us to reuse a previously closed slot. > > The current fix is to call PK11_InitToken in the pk11wrap layer. > It seems that ideally the fix should be contained in the softoken, > perhaps by changing SFTK_ShutdownSlot or SFTK_SlotReInit to do > more cleanup when we close or reopen a user DB. Is that possible? No. softoken/SFTK_/sftk_ have no knowledge/interaction with the PKCS#11 wrapper layer (PK11SlotInfo). PKCS#11 sits higher than softoken. Layer wise, we have: SECMOD_ nssToken_ PK11_ (sftk_) Softoken is properly re-initializing all of its slot state variables. Likewise, the PKCS#11 layer, if used as a PKCS#11 slot, also properly handles re-initializing all of its variables. The problem comes from the fact that SECMOD_ is using the token both through the nssToken_ interface /and/ through the PK11_ slot interface, and so there are two connections that have to be re-initialized. The existing code only re-initializes the nssToken_ interface, because the "normal" use of SECMOD_ is implemented in terms of nssToken. Technically, this problem exists with any PKCS#11 token that is exposed both as an NSS token and as a PKCS#11 token - which is only SECMOD_ code. If there was other code dually exposed, then the included fix would not be sufficient, since code /may/ not know when a token eject/insert happens in a slot. My Comment 6 was about how such events should be handled, if such scenarios seem reasonable.
Comment on attachment 643486 [details] [diff] [review] Patch 3: Address wtc's comments Review of attachment 643486 [details] [diff] [review]: ----------------------------------------------------------------- r=wtc. Please wait for Bob's review. Thanks! There is a very incomplete NSS Coding Style document at http://www.mozilla.org/projects/security/pki/nss/devel/coding-style.html Just imitate the *local* style of the code you're modifying as much as you can. I can fix the style nits for you when I check in your patches. I usually don't fix style nits such as tabs vs. spaces unless I need to modify that code for other reasons. Some other NSS developers such as Nelson tend to be more aggressive in fixing style issues. ::: mozilla/security/nss/cmd/tests/secmodtest.c @@ +26,5 @@ > + fprintf(stderr, "Usage: %s -d dbDir\n", progName); > + exit(1); > +} > + > + Remove a blank line. ::: mozilla/security/nss/lib/pk11wrap/pk11util.c @@ +1303,5 @@ > PRBool present = PK11_IsPresent(slot); > PK11_FreeSlot(slot); > if (present) { > return PR_FALSE; > } Nit: we can also define this function to only set *slotExists when it returns PR_TRUE. If so, we should move the "*slotExists = PR_TRUE;" here. ::: mozilla/security/nss/lib/softoken/pkcs11.c @@ +3908,5 @@ > static CK_RV sftk_CreateNewSlot(SFTKSlot *slot, CK_OBJECT_CLASS class, > SFTKObject *object) > { > + PRBool isValidUserSlot = PR_FALSE; > + PRBool isValidFIPSSlot = PR_FALSE; Nit: isValidFIPSSlot => isValidFIPSUserSlot
Attachment #643486 - Flags: review+
Comment on attachment 643486 [details] [diff] [review] Patch 3: Address wtc's comments r+ for the secmodtest.c and changes to softoken. The changes to pk11util worry me. You shouldn't have to call PK11_TokenInit. That should happen automatically under the following mechanism: All user slots are marked as 'removable slots', which means the slot looks like a 'smart card' reader. In a smart card reader you can have a smart card inserted do some stuff and then remove the smart card and insert another one. NSS detects this insertion/removal automatically with 2 mechanisms: 1a. If NSS detects that a token that was present, is no longer, it marks the slot as empty (isPresent == PR_FALSE). 1b. If NSS detects taht a token that wasn't present now is present, it calls PK11_TokenInit(). 2. When NSS checks to see if a token is present, if the token is removable, NSS checks to see if the global session it allocates in PK11_TokenInit() is still valid. If that session isn't valid, but the PKCS #11 module still marks the token as present, it presumes that the previous token has been has been removed and a potentially new token has been inserted, so PK11_TokenInit() is called. The code you have to try to force the call to PK11_TokenInit means one of these mechanisms has gone wire, and we probably should fix the underlying problem. The issues could be: 1) Softoken isn't properly marking the USER tokens as removable. 2) Softoken isn't properly clearing all the sessions. 3) Softoken isn't properly failing when called with a cleared session in #2. 4) PK11_IsPresent() isn't properly checking the session. 5) Somehow you've hit a path where PK11_IsPresent isn't called on the slot before you try to use it. If the problem is #5, then the better solution would be to change the last line of SECMOD_OpenNewSlot() from return SECMOD_FindSlotByID(mod, slotID); To slot = SECMOD_FindSlotByID(mod, slotID); /* make sure the slot is active as soon as we exit */ if (slot) { (void) PK11_IsPresent(slot); } return slot;
Attachment #643486 - Flags: review?(rrelyea) → review-
> 2) Softoken isn't properly clearing all the sessions. Sofoken isn't properly clearing all the sessions on delete.
(In reply to Robert Relyea from comment #12) > Comment on attachment 643486 [details] [diff] [review] > Patch 3: Address wtc's comments > > r+ for the secmodtest.c and changes to softoken. > > The changes to pk11util worry me. You shouldn't have to call PK11_TokenInit. > That should happen automatically under the following mechanism: > > All user slots are marked as 'removable slots', which means the slot looks > like a 'smart card' reader. In a smart card reader you can have a smart card > inserted do some stuff and then remove the smart card and insert another > one. NSS detects this insertion/removal automatically with 2 mechanisms: > > 1a. If NSS detects that a token that was present, is no longer, it marks the > slot as empty (isPresent == PR_FALSE). > 1b. If NSS detects taht a token that wasn't present now is present, it calls > PK11_TokenInit(). > > 2. When NSS checks to see if a token is present, if the token is removable, > NSS checks to see if the global session it allocates in PK11_TokenInit() is > still valid. If that session isn't valid, but the PKCS #11 module still > marks the token as present, it presumes that the previous token has been has > been removed and a potentially new token has been inserted, so > PK11_TokenInit() is called. > > The code you have to try to force the call to PK11_TokenInit means one of > these mechanisms has gone wire, and we probably should fix the underlying > problem. The issues could be: > > 1) Softoken isn't properly marking the USER tokens as removable. > 2) Softoken isn't properly clearing all the sessions. > 3) Softoken isn't properly failing when called with a cleared session in #2. > 4) PK11_IsPresent() isn't properly checking the session. > 5) Somehow you've hit a path where PK11_IsPresent isn't called on the slot > before you try to use it. This is explained in Comment #6. When the slot is an NSS token (that is, it's the SECMOD token), then the PKCS#11 layer is semi-bypassed/wrapped by the nssTrust layer. That is, rather than directly interacting with the SECMOD DB via PKCS#11, most interactions are mediated by the nssTrust layer.The nssTrust layer properly re-establishes the PKCS#11 layer (as described in Comment #6), but the OpenUserDB/CloseUserDB doesn't interact with nssTrust, and that's why it gets in trouble. Bob, how do you feel about the alternative proposed in comment #6 - which is to ensure in PK11_IsPresent that /both/ the nssTrust AND the PKCS#11 layers are re-initialized? The existing code suggests that the "nssTrust only" was an intentional optimization, which is why I made the SECMOD-aware changes instead. > > If the problem is #5, then the better solution would be to change the last > line of SECMOD_OpenNewSlot() from > > return SECMOD_FindSlotByID(mod, slotID); > To > > slot = SECMOD_FindSlotByID(mod, slotID); > /* make sure the slot is active as soon as we exit */ > if (slot) { > (void) PK11_IsPresent(slot); > } > return slot;
nssToken_IsPresent() should use exactly the same technique as PK11_IsPresent. I believe in the current NSS system nssToken_isPresent is always used. If there is a problem, it would also occur for smart cards as well.
Ah ha, The problem is with within_token_delay_period(slot) We need to reset the delay period. The following code will accoplish this: /* if we are in the delay period for the "isPresent" call, reset * the delay since we know things have probably changed... */ if (slot && slot->nssToken && slot->nssToken->slot) { nssSlot_ResetDelay(slot->nssToken->slot); } You'll see the same code in SECMOD_WaitForAnyTokenEvent(). bob
(In reply to Robert Relyea from comment #16) > Ah ha, The problem is with > > within_token_delay_period(slot) > > We need to reset the delay period. The following code will accoplish this: > > /* if we are in the delay period for the "isPresent" call, reset > * the delay since we know things have probably changed... */ > if (slot && slot->nssToken && slot->nssToken->slot) { > nssSlot_ResetDelay(slot->nssToken->slot); > } > > > You'll see the same code in SECMOD_WaitForAnyTokenEvent(). > > bob Hi Bob, No, that's not the problem. As mentioned in comment #6, the issue is at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11slot.c&rev=1.113&mark=1393,1409-1410,1429,1435,1445-1448#1387 Specifically, lines 1409/1410 cause the token detection to short-circuit if there is an associated NSS trust setting (which there is for SECMOD-based databases). I've tried to highlight the problematic areas in the above. You can see that based on line 1409-1410 causes all of the highlighted code below (1429, 1435, 1445-1148) to be skipped.
Ryan, I think are are misreading the code. The slot->nssToken does not mean the slot is trusted. In modern NSS slot->nssToken is always set. It is the so called "Stan" implementation. No code you point out has anything to do with trust (there is not nssTrust element in the slot).. All the code below 1413 is effectively dead code. What is happening is we are calling nssToken_IsPresent(), which immediately calls nssSlot_IsTokenPresent(). Line 139 of dev/devslot.c has a line which checks to see if we've checked the status of the token recently. This is because we call IsPresent *A LOT*. For most smart cards it can be expensive to query the state, so we return our last answer if we've been pinged in the last second. Some operations we know indicate a true state change (like WaitForTokenEvent, or creating a new slot under software control). We need to reset our time so that the very next IsPresent call will work. The fragment in Comment 16 affects the behavior of nssToken_IsPresent() specifically. bob
(In reply to Robert Relyea from comment #18) > Ryan, I think are are misreading the code. > > The slot->nssToken does not mean the slot is trusted. In modern NSS > slot->nssToken is always set. It is the so called "Stan" implementation. > > No code you point out has anything to do with trust (there is not nssTrust > element in the slot).. All the code below 1413 is effectively dead code. > > What is happening is we are calling nssToken_IsPresent(), which immediately > calls nssSlot_IsTokenPresent(). Line 139 of dev/devslot.c has a line which > checks to see if we've checked the status of the token recently. This is > because we call IsPresent *A LOT*. For most smart cards it can be expensive > to query the state, so we return our last answer if we've been pinged in the > last second. Some operations we know indicate a true state change (like > WaitForTokenEvent, or creating a new slot under software control). We need > to reset our time so that the very next IsPresent call will work. The > fragment in Comment 16 affects the behavior of nssToken_IsPresent() > specifically. > > bob Thanks for the explanation Bob. I'm still not sure it's fully capturing the bug though: slot->nssToken maintains its own PKCS#11 connection. This connection /is/ successfully re-established. I understand your responses about how to make slot->nssToken re-initialize itself - but it already is being correctly re-initialized, so that's not the bug. The bug is that the SECMOD_ functions use slot->session to interact with the PKCS#11 layer, not slot->nssToken->slot . Because of the nssToken short-circuit on 1409/1410, slot->nssToken->slot /is/ successfully re-initialized, but without the PK11_InitSlot call, slot->session is not initialized. That's why the explicit call was added. Other calls that are using slot->session are either re-initializing the session themselves (ie: duplicating the PK11_IsPresent / C_GetSessionInfo call) or are similarly bugged in handling removable slots. The access to slot->session specifically happens at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11util.c&rev=1.63&mark=1266,1282-1283#1266 I guess the question is which is the 'authoritative' PKCS#11 session - slot->session or slot->nssToken->session. It sounds like the latter?
The sessions are supposed to be identical. A little history: Sometime back in 2000, we embarked on an API cleanup on NSS which was going to the NSS 4.0, also known as "Stan". The cleanup involved the certificate processing and low level token processing. We planned several phases: First put stan code in and implement the old API on top of stan. Expose the stan interface. Eventually deprecate our old API. The problem is after we started, no users wanted to rewrite their apps to use the API. since the users funded NSS developement, NSS 4.0 never got passed the first phase. The PK11/SECMOD is the 'old' API, and nssToken nssSlot and friends are the new API. The nssToken and nssSlot is important because the cert processing code in pki uses the NSS 4.0 interfaces internally. In order for things to work properly, we needed to mirror certain elements that were unforuntately made visible in NSS3 to applications, namely elements in PK11Slot and CERTCertificate. Most of the code that handles this mapping is pki/pki3hack.c and pk11wrap/dev3hack.c. Now that you have the foundation, I can explain how things are supposed to work. At initialization time, we still kick things off by creating PK11_SlotInfos. As part of creating a PK11SlotInfo, we also create a NSSSlot and NSSToken to go with it. The NSSToken is initialized with a Session matching the session of the PK11SlotInfo. So the session in the NSSToken is identical to the session in the slotinfo. Now when we call nssToken_IsPresent() (See dev/devtoken.c), it calls nssSlot_TokenIsPresent() (see deve/devslot.c). The nssSlot_TokenIsPresent() makes several checks: 1) is perm - which always returns the true if the slot is enabled (Note the PK11_ API creeping into devslot -- this is probably a bug, and should abstracted out so the PK11_ call is really in pk11wrap/dev3hack.c). 2) if we are within the ispresent delay period, we return the last state we read. NOTE: the delay period is 1 second. If you have called any NSS functions on the old slot in the last second this function will short circuit all the rest of the code. This also means you may run into the issue where it fails when you run the code, but succeeds when you step through it in the debugger! 3) we use the session (which is identical to the session in the slot). NOTE: when I say 'identical' I mean they both have the same handles. Be careful of the variable names here, in the pk11wrap code, the session is a naked PKCS#11 session handle. In the devslot/devtoken code it is a reference counted data structure that includes the naked PKCS #11 session handle. Those two session handles are what is identical, not the value of the variable 'session'. 4) if that session is now invalid (either because the slot is empty are a new token has been inserted in the slot), The code call nssSlot_Refresh() 5) In nssSlotRefresh (see pk11wrap/dev3hack.c), We call PK11_TokenInit() on the PK11SlotInfo). We then refresh the session handle in the NSSToken to match the new session handle from the PK11SlotInfo. The upshot: 1. If you really are successfully initializing slot->nssToken->slot, then you are also properly initializing slot. This means your statement that slot->nssToken->slot *is* successfully re-initialized is not correct. I presume you made this mistake because you traced the code in the debugger, in which case the wait_for_token_delay() was not active (it's pretty easy to burn a full second in the debugger. 2. The only possible ways that nssSlot_Refresh is not being called are: a. the token is perm. b. we are in the timeout period. c. C_GetSlotInfo() is returning CKF_TOKEN_PRESENT bit in the flags as zero. d. C_GetSessionInfo() is returning success on the session handle in the token. I'm pretty sure we are dealing with case 2b. (I've seen this kind of error before when dealing with slot events. I suggest trying my patch, and if it fails, then we can come back and find out which of the other conditions in 2 must have caused it. bob
(In reply to Robert Relyea from comment #12) > Comment on attachment 643486 [details] [diff] [review] > Patch 3: Address wtc's comments > > r+ for the secmodtest.c and changes to softoken. > > The changes to pk11util worry me. You shouldn't have to call PK11_TokenInit. > That should happen automatically under the following mechanism: > > All user slots are marked as 'removable slots', which means the slot looks > like a 'smart card' reader. In a smart card reader you can have a smart card > inserted do some stuff and then remove the smart card and insert another > one. NSS detects this insertion/removal automatically with 2 mechanisms: > > 1a. If NSS detects that a token that was present, is no longer, it marks the > slot as empty (isPresent == PR_FALSE). > 1b. If NSS detects taht a token that wasn't present now is present, it calls > PK11_TokenInit(). > > 2. When NSS checks to see if a token is present, if the token is removable, > NSS checks to see if the global session it allocates in PK11_TokenInit() is > still valid. If that session isn't valid, but the PKCS #11 module still > marks the token as present, it presumes that the previous token has been has > been removed and a potentially new token has been inserted, so > PK11_TokenInit() is called. > > The code you have to try to force the call to PK11_TokenInit means one of > these mechanisms has gone wire, and we probably should fix the underlying > problem. The issues could be: > > 1) Softoken isn't properly marking the USER tokens as removable. > 2) Softoken isn't properly clearing all the sessions. > 3) Softoken isn't properly failing when called with a cleared session in #2. > 4) PK11_IsPresent() isn't properly checking the session. > 5) Somehow you've hit a path where PK11_IsPresent isn't called on the slot > before you try to use it. > > If the problem is #5, then the better solution would be to change the last > line of SECMOD_OpenNewSlot() from > > return SECMOD_FindSlotByID(mod, slotID); > To > > slot = SECMOD_FindSlotByID(mod, slotID); > /* make sure the slot is active as soon as we exit */ > if (slot) { > (void) PK11_IsPresent(slot); > } > return slot; Your patch does not resolve this issue. The problem still exists. This is supporting the statement that the issue is #4 from this list. Because of the forwarding to nssToken_IsPresent, slot->session is not re-initialized by PK11_IsPresent. #0 nssSlot_IsTokenPresent (slot=0x806dd30) at devslot.c:181 #1 0xf7e89f97 in nssToken_IsPresent (token=0x806d528) at devtoken.c:1445 #2 0xf7e61da6 in pk11_IsPresentCertLoad (slot=0x805c558, loadCerts=1) at pk11slot.c:1410 #3 0xf7e61f91 in PK11_IsPresent (slot=0x805c558) at pk11slot.c:1458 #4 0xf7e6567d in secmod_SlotIsEmpty (mod=0x80514f8, slotID=4, slotExists=0xffffcb58) at pk11util.c:1303 #5 0xf7e65714 in secmod_FindFreeSlot (mod=0x80514f8, slotExists=0xffffcb58) at pk11util.c:1334 #6 0xf7e65772 in SECMOD_OpenNewSlot (mod=0x80514f8, moduleSpec=0x805ca28 "configDir='/tmp/test1' tokenDescription='Test Slot 2'") at pk11util.c:1366 #7 0xf7e658f8 in SECMOD_OpenUserDB (moduleSpec=0x805ca28 "configDir='/tmp/test1' tokenDescription='Test Slot 2'") at pk11util.c:1479 #8 0x080492ef in TestOpenCloseUserDB (progName=0xffffce89 "secmodtest", configDir=0x804f488 "/tmp/test1", tokenName=0x804b1d5 "Test Slot 2") at secmodtest.c:45 #9 0x080494ea in main (argc=3, argv=0xffffccc4) at secmodtest.c:105 Code in question: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/dev/devslot.c&rev=1.28&mark=124,149,157-172#124 This causes nssSlot_IsTokenPresent to re-initialize the nssSession (from line 162) on line 166. Before 162-172: [Inside nssSlot_IsTokenPresent ] (gdb) print *session $39 = { lock = 0x805c478, handle = 33554433, slot = 0x806dd30, isRW = 0, ownLock = 0 } [Inside PK11_IsPresent] (gdb) print slot $43 = (PK11SlotInfo *) 0x805c558 (gdb) print slot->session $44 = 33554433 [Afterwards] (gdb) print *session $48 = { lock = 0x805c478, handle = 0, slot = 0x806dd30, isRW = 0, ownLock = 0 } (gdb) print slot->session $50 = 33554433 The issue is this: nssSlot_IsTokenPresent re-initializes slot->nssToken->slot->[session]->handle It does not re-initialize slot->session nor slot->nssToken->slot->session (which point to the same memory) The SECMOD functions use slot->session directly ( this is http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11util.c&rev=1.63&mark=1266,1282-1283#1266 ). When it fails, slot->session is not re-initialized. Does that clarify the comments about them using different sessions? Note, these are CK_HANDLE sessions, not nssSessions being shared. When the slot is ejected and re-inserted, the CK_HANDLE hashtable on the underlying sftk_ slot is cleared, thus regardless of ref-counting, all the old sessions are legitimately invalidated.
> Code in question: > http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/dev/devslot.c&rev=1.28& > mark=124,149,157-172#124 > > This causes nssSlot_IsTokenPresent to re-initialize the nssSession (from line 162) on line 166. Nope Not even close. This code is *INVALIDATING* a session, there is no initialization happening here. Is this where you see the code after you initialize softoken? If so the problem is that softoken, for some reason, does not believe the token is really present. NOTE also, If you ever get to this code, then you are extremely unlikely to get to any other part of pk11wrap with this token because the token will be marked removed. > Does that clarify the comments about them using different sessions? I understand how pk11wrap uses the session, and how NSSToken is supposed to keep them in sync. Yes, there is a corner case where one could have an invalid handle and the other have a handle that is CK_INVALID_HANDLE. You are still reading things into the code that isn't there. bob
Hi Bob, I suspect I'm not explaining things correctly, so I apologize for any miscommunication. Yes, the code is invalidating the session. That serves as the necessary step for the initialization logic that will happen at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/dev/devslot.c&rev=1.28&mark=199,205-217#199 on the next call to PK11_IsPresent via nssSlot_IsTokenPresent. CK_INVALID_SESSION is the global signal that the slot is either (newly created, removed and re-inserted). That's why I considered it part of initialization - it's the initial state of the slot when PK11_InitSlot/PK11_InitToken is called. Language choice of init vs shutdown aside, hopefully the problem itself is clear: slot->session refers to a session handle != CK_INVALID_SESSION. Because of this, code that uses CK_INVALID_SESSION as a signal to establish a new session (namely, either nssSlot_IsTokenPresent as pasted above, or PK11_IsPresent at http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11slot.c&rev=1.113&mark=1393,1442-1448#1442 ) To rephase the question about the approach: Should nssToken_IsPresent should be responsible for updating slot->pk11slot->session in addition to slot->token->defaultSession . These session handles are not equivalent, and are not currently being kept in sync, as shown through the unit test and debugging.
>> >> If the problem is #5, then the better solution would be to change the last >> line of SECMOD_OpenNewSlot() from >> >> return SECMOD_FindSlotByID(mod, slotID); >> To >> >> slot = SECMOD_FindSlotByID(mod, slotID); >> /* make sure the slot is active as soon as we exit */ >> if (slot) { >> (void) PK11_IsPresent(slot); >> } >> return slot; >> > Your patch does not resolve this issue. The problem still exists. This wasn't the patch I was refering to, I was refering to the code in comment 16. I now believe you probably need both.
Ryan, you are looking at the wrong cave. Everything you are pointing out are red herrings. > To rephase the question about the approach: Should nssToken_IsPresent should be responsible for > updating slot->pk11slot->session in addition to slot->token->defaultSession . These session > handles are not equivalent, and are not currently being kept in sync, as shown through the unit > test and debugging. The answer is a qualified yes. In the code you point out, there is no reason to worry about the out-of-syncness. The two handles in both those cases are invalid. Anyone using them would get the same result (other then direct comparison). The only way for there to be valid handles is nssSlot_Refresh() to be called, which syncs the handles itself. The bug is nssSlot_Refresh is not being called.
OK, I've taken a look at the sample test. There appears to be 2 issues: 1) It appears that none of the underlying NSS functions except SECMOD_CloseUserDB() call PK11_IsPresent(). [The issue I raised in comment 12 ]. 2) There is clearly less than a second between the SECMOD_ClosedUserDB(), and the next PK11_IsPresent() call after the SECMOD_OpenUserDB returns the next open slot. [The issue I pointed out in comment 16 ] If I install your patch to softoken, and make the following modification to your test code: + include "unistd.h" int TestUserDB(char* userdbdir) { PK11SlotInfo* db_slot = NULL; CERTCertificate* cert = NULL; CERTCertTrust trust = {0}; char* nickname = NULL; char modspec[256]; SECItem der_cert; int rv = 0; sprintf(modspec, "configDir='sql:%s' tokenDescription='%s'", userdbdir, userdbdir); printf("opening userdb: %s\n", modspec); db_slot = SECMOD_OpenUserDB(modspec); if (!db_slot) { fprintf(stderr, "SECMOD_OpenUserDB failed: %i\n", PORT_GetError()); return -1; } + sleep(2); printf("token name:%s slot name:%s\n", PK11_GetTokenName(db_slot), PK11_GetSlotName(db_slot)); + if (!PK11_IsPresent(db_slot)) { + fprintf(stderr, "Slot is not present\n"); + goto loser; + } Then the tests run without error. Clearly requiring the sleep() and the IsPresent() call in the application is wrong, we do need to modify SECMOD_OpenNewSlot() with both the code from comment 12 and the code from comment 16.
Here's a working patch for pk11util.c Index: pk11util.c =================================================================== RCS file: /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v retrieving revision 1.63 diff -r1.63 pk11util.c 1396c1396,1407 < return SECMOD_FindSlotByID(mod, slotID); --- > slot = SECMOD_FindSlotByID(mod, slotID); > if (slot) { > /* if we are in the delay period for the "isPresent" call, reset > * the delay since we know things have probably changed... */ > if (slot->nssToken && slot->nssToken->slot) { > nssSlot_ResetDelay(slot->nssToken->slot); > } > /* force the sto info structures to properly reset */ > (void) PK11_IsPresent(slot); > } > > return slot;
Thanks for the clarifications, Bob, and sorry it took so long for the lighbulb to come on. What was missing was that slot->nssToken->defaultSession->handle and slot->session are kept in sync via nssToken_Refresh, which is called by nssSlot_Refresh. Rather than directly modifying defaultSession->handle to update it to the (new) value of slot->session, it's creating a new nssSession entirely via nssSession_ImportNSS3Session. Incidentally, when looking at the codepath being taken, ISTM that slot->nssToken->defaultSession is not being freed before being re-initialized via nssToken_Refresh. While nssSession_ImportNSS3Session is allocating the nssSession within the nssSlot's arena, the fact that the old nssSession is not freed before-hand suggests that memory allocations will continue to grow as more slots are added/removed. I don't see nssSlot_Destroy (which ultimately is responsible for free'ing the nssArena, thus freeing all of the nssSessions) being called until STAN_Shutdown(). Does http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/dev3hack.c&rev=1.27&mark=225,233-237#225 look correct to you? Have I missed something? Breakpoint 2, nssToken_Refresh (token=0x806d528) at dev3hack.c:229 229 if (!token) { (gdb) print token->defaultSession $2 = (nssSession *) 0x80566e8 Breakpoint 3, nssToken_Refresh (token=0x806d528) at dev3hack.c:233 233 token->defaultSession = (gdb) print token->defaultSession $3 = (nssSession *) 0x80566e8 (gdb) n 238 return token->defaultSession ? PR_SUCCESS : PR_FAILURE; (gdb) print token->defaultSession $4 = (nssSession *) 0x806dd80 Should I also address that in this patch, or would you prefer a separate one-liner to call nssSession_Destroy in nssToken_Refresh?
If the nssSession is allocated out of the nssslot arena, there really isn't a way to free it until we free the slot. slowly growing based on token insertion/removals is probably bad:), could you write a separte bug about this. I think long term we'll need too alloc/free the nssSession on it's own and not share the NSSSlot arena. bob
Thanks for the assistance. Attached is the 'correct' fix.
Attachment #643486 - Attachment is obsolete: true
Attachment #649492 - Flags: review?(rrelyea)
Comment on attachment 649492 [details] [diff] [review] Complete patch with slot re-initialization r+ rrelyea Though it looks like the indent formatting got squashed in the pk11util.c part of the patch. Just make sure it's correct. bob
Attachment #649492 - Flags: review?(rrelyea) → review+
Attached patch Corrected indentation (obsolete) — — Splinter Review
Bob or Wan-Teh, would you mind committing this? The only difference here with the previous patch is the corrected indentation that Bob pointed out.
Attachment #649492 - Attachment is obsolete: true
Attachment #651012 - Flags: review?(wtc)
I spotted some indentation problems in sftk_CreateNewSlot. I fixed the indentation and check this in on the NSS trunk (NSS 3.14). Checking in cmd/tests/manifest.mn; /cvsroot/mozilla/security/nss/cmd/tests/manifest.mn,v <-- manifest.mn new revision: 1.12; previous revision: 1.11 done RCS file: /cvsroot/mozilla/security/nss/cmd/tests/secmodtest.c,v done Checking in cmd/tests/secmodtest.c; /cvsroot/mozilla/security/nss/cmd/tests/secmodtest.c,v <-- secmodtest.c initial revision: 1.1 done Checking in lib/pk11wrap/pk11util.c; /cvsroot/mozilla/security/nss/lib/pk11wrap/pk11util.c,v <-- pk11util.c new revision: 1.64; previous revision: 1.63 done Checking in lib/softoken/pkcs11.c; /cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v <-- pkcs11.c new revision: 1.181; previous revision: 1.180 done
Attachment #651012 - Attachment is obsolete: true
Attachment #651012 - Flags: review?(wtc)
Comment on attachment 651614 [details] [diff] [review] Patch by Ryan Sleevi (as checked in) Review of attachment 651614 [details] [diff] [review]: ----------------------------------------------------------------- Bob,Ryan: I have two questions about your change to SECMOD_OpenNewSlot. ::: mozilla/security/nss/lib/pk11wrap/pk11util.c @@ +1398,5 @@ > + /* if we are in the delay period for the "isPresent" call, reset > + * the delay since we know things have probably changed... */ > + if (slot->nssToken && slot->nssToken->slot) { > + nssSlot_ResetDelay(slot->nssToken->slot); > + } I know this code comes from: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/security/nss/lib/pk11wrap/pk11util.c&rev=1.63&mark=1165-1169#1165 Why are the null pointer tests for slot->nssToken and slot->nssToken->slot necessary? Can they be null? @@ +1400,5 @@ > + if (slot->nssToken && slot->nssToken->slot) { > + nssSlot_ResetDelay(slot->nssToken->slot); > + } > + /* force the slot info structures to properly reset */ > + (void)PK11_IsPresent(slot); Please confirm that this PK11_IsPresent call is necessary. Why don't we need to call PK11_IsPresent in SECMOD_WaitForAnyTokenEvent? (See the Bonsai link I provided above.)
> Why are the null pointer tests for slot->nssToken and > slot->nssToken->slot necessary? Can they be null? Paranoia. It know for sure that they can't be null would require a pretty extensive review of the PK11SlotInfo usage. I'm pretty sure it should be, but the code is riddled with 'hedging our bets'. What I can say is if it's not NULL, then the nssSlot_ResetDelay isn't necessary because the PK11_IsPresent function will operate without calling the nssToken_IsPresent (which calls nssSlot_IsPresent which has the delay). > Please confirm that this PK11_IsPresent call is necessary. It seems to be. Certainly the test case Ravi has manages to call functions that operate on the token without first calling PK11_IsPresent. For completeness, SECMOD_WaitForAnyTokenEvent may need the PK11_IsPresent call as well, except in normal usage the caller of SECMOD_WaitForAnyTokenEvent almost always calls PK11_IsPresent() immediately (the token event could be insertion, removal or removal/insertation. You don't know until you call PK11_IsPresent(). bob
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: