138084 - NSC_Initialize leaves a slot list even if it fails

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

Consider the following test program :

#include "nss.h"

int  main(int argc, char** argv)
{
    SECStatus rv = NSS_Initialize("1", "2", "3", "secmod.db", 
                        NSS_INIT_READONLY);
    if (SECSuccess != rv)
    {
        rv = NSS_NoDB_Init(".");
    }
    NSS_Shutdown();
}

In this case, the program attempts to initialize NSS with NSS_Initialize(). If 
it doesn't find its cert/key/secmod databases, it will revert to using 
NSS_NoDB_Init().

The problem occurs during shutdown. There is an assertion in NSC_Finalize in 
softoken. Debugging shows that there are 4 slots in the slot list instead of 2 
as there should be. Further investigation shows that


- NSC_Initialize got called twice during NSS_Initialize. SECMOD_LoadPKCS11Module 
does 2 attempts to load a module, one with thread safety and one without.

  213       if (PK11_GETTAB(mod)->C_Initialize(&secmodLockFunctions) != CKR_OK) 
{
  214           mod->isThreadSafe = PR_FALSE;
  215           if (PK11_GETTAB(mod)->C_Initialize(NULL) != CKR_OK) goto fail;
  216       }

Both times, the crypto slot was added to the slot list, and the internal db slot 
failed to be added

- NSC_Initialize got called a third time from NSS_NoDB_Init . At that point, 
both slots were added properly (the db slot is an empty slot), filling slots 3 
and 4.

The fix is to destroy the entire softoken slot list if any slot fails to be 
added within NSC_Initialize.

Comment 1

[Julien Pierre]

Reassigning to self

Comment 2

[Julien Pierre]

Attachment: Fix to clean up slot list on NSC_Initialize failure

Comment 3

[Julien Pierre]

Bob,
I looked at the other possible error cases related to that list. I didn't see 
anything else that could cause the shutdown code to go bad.

I did find a potential memory leak in an error case though, due to a realloc().
This is a diff of my fix for it. I'd like to check it in at the same time as the 
patch I attached, which hasn't changed.

*** 2076,2085 ****
        }
      }
      if (nscSlotCount >= nscSlotListSize) {
        nscSlotListSize += NSC_SLOT_LIST_BLOCK_SIZE;
!       nscSlotList = (CK_SLOT_ID *) PORT_Realloc(nscSlotList,
                                        nscSlotListSize*sizeof(CK_SLOT_ID));
        if (nscSlotList == NULL) {
            return NULL;
        }
      }
--- 2076,2089 ----
        }
      }
      if (nscSlotCount >= nscSlotListSize) {
+         CK_SLOT_ID* oldNscSlotList = nscSlotList;
+         CK_ULONG oldNscSlotListSize = nscSlotListSize;
        nscSlotListSize += NSC_SLOT_LIST_BLOCK_SIZE;
!       nscSlotList = (CK_SLOT_ID *) PORT_Realloc(oldNscSlotList,
                                        nscSlotListSize*sizeof(CK_SLOT_ID));
        if (nscSlotList == NULL) {
+             nscSlotList = oldNscSlotList;
+             nscSlotListSize = oldNscSlotListSize;
            return NULL;
        }
      }

Comment 4

[Julien Pierre]

I checked in the fix to both NSS_3_4_BRANCH and the tip.