67068 - Need a SECMOD_Shutdown

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Javier Delgadillo]

This is related to Bug 66939.

Client embedding groups are supporting switching profiles without exiting the 
application.  NSS 3.2 uses a loadable PKCS11 module for its list of trusted 
roots.  When shutting down NSS, that shared library is not unloaded and then the 
next time NSS is initialized, the PKCS11 libraries fail to initialize because 
the root certs module was never un-loaded.  

PSM bundles the PKCS11 module in the same directory as the executable because we 
can't guess the user profile directory before run-time, so every profile that 
gets run will be pointed to the same shared library.

I worked around this by calling SECMOD_DeleteModule for the loadable root certs 
module before shutting down NSS.  In order to make smart cards useable with such 
products in the future, we need to have NSS shut down the PKCS11 libraries when 
we shut down NSS.

Comment 1

[Javier Delgadillo]

I should add that implementing SECMOD_Shutdown and then calling it from within 
NSS_Shutdown would solve this problem as well.

Comment 2

[Javier Delgadillo]

I just did some more investigation and realized PSM 2.0 will need this in NSS3.2

What happens is that the SECMOD libraries keep a global modules variable around.  
When you try to re-initialize NSS, the modules variable is already defined so 
the new secmod.db is not read/created.

This will mean any embedding app that uses PSM 2.0 will only use the first 
semod.db that was loaded for all profiles that get launced.  That may be OK, but 
is that we want?

Comment 3

[Robert Relyea]

I can work on a SECMOD_Shutdown, but

1) I'm not sure we can support a full nss shutdown and restart in the 3.2 time
frame (Mostly because NSS has never been designed to shutdown and restart. This
is a Feature request that needs to be started *BEFORE* we ship betas). 3.2 RTM's
in 2 weeks.
2) You definately won't be able to run two different profiles at the same time.
That is a Stan feature which requires a line in a PRD and a major release to
fulfill.

Comment 4

[Javier Delgadillo]

We don't want to run 2 profiles at the same time.  We need to shut one down, and 
the initialize another one.  cert and key db's are currently working on the Mac, 
it's just secmod that can't handle another initialization.

Comment 5

[Conrad Carlen (not reading bugmail)]

Right, we certainly don't need two profiles at the same time.
In terms of the time frame, note that this blocks bug 64833. I'm sorry about the
short notice, but at some point in the past month or so, this was working with
profile switching and the problem showed up farily recently. 

Comment 6

[Robert Relyea]

Attachment: Implement NSS_Shutdown()

Comment 7

[Robert Relyea]

wtc can you review the patch.
javi. can you try the patch in psm and see if it does in fact allow NSS to be
called and initialized again.

bob

Comment 8

[Javier Delgadillo]

It'll be a day or so before I can get around to applying and testing this patch.

Comment 9

[Wan-Teh Chang]

Bob, your patch is good.  By the way, the code in SECMOD_Shutdown
seems to be indented with a tab (or 8 spaces).

Comment 10

[Robert Relyea]

Oops. That's now fixed.

BTW, This code assumes that NSS calls have quiessed.... that is if you have an
open encryption or SSL session still running unexpected behavior may result.

Nelson, is it necessary to flush the SSL cache as well (I think it's safest if
you do, and after shutdown nothing in the cashe will be usable as the empheral
wrapping key will have disappeared when the internal token gets closed.

Comment 11

[Wan-Teh Chang]

Nelson, Bob has a question for you in this bug report.

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Yes, The client must call SSL_ClearSessionCache()
any time it changes the SSL configuration (e.g. the 
enabled ciphersuites, or versions of SSL/TLS, or if
it wants to shutdown NSS.

Comment 13

[Robert Relyea]

OK, SDR wasn't happy last night. Turns out there were a couple of potential
problems: 1) NSS_Shutdown can be called in the middle of NSS_Init if the db's
don't initialize. SECMOD_Init is now tolerant of being called even if the
security module hasn't been initialized.  2) The keys on the slot free list do
not have a slot value in them (since we've freed the reference. Everything
handled this case except when we freed up the slot, when we go to destroy all
the keys were were trying to use symkey->slot which doesn't exit (this one was
the real crash in sdrtest).

Also NOTE: sdrtest does '-d' does not work. I have a separate patch for this
which I'll file under a separate bug.

Comment 14

[Robert Relyea]

Attachment: Fix Shutdown crashes... attach patch to the correct bug..

Comment 15

[Nelson Bolyard (seldom reads bugmail)]

Instead of eliminating the call to SECU_ConfigDirectory in 
case 'd',
why not change the NSS_Init call to 
<    rv = NSS_Init(certDir);
>    rv = NSS_Init(SECU_ConfigDirectory(NULL));

Comment 16

[Robert Relyea]

I also wanted to do the strdup of the parameters as well. With the new
Initialize sequence the SetConfigDirectory stuff really isn't needed anymore.

Comment 17

[Robert Relyea]

Attachment: ARGGE THIS is the correct patch!

Comment 18

[Robert Relyea]

Attachment: Third times a charm...

Comment 19

[Wan-Teh Chang]

The stack trace in the core file generated by sdr.sh is:
(dbx on Solaris 2.6)

detected a multithreaded program
dbx: program is not active
t@1 (l@1) terminated by signal SEGV (no mapping at the fault address)
Current function is SECMOD_DestroyListLock
   73       PK11_USE_THREADS(PZ_DestroyMonitor(lock->monitor);)
(dbx) print lock                                                             
lock = (nil)
(dbx) where    
current thread: t@1
=>[1] SECMOD_DestroyListLock(lock = (nil)), line 73 in "pk11list.c"
  [2] SECMOD_Shutdown(), line 179 in "pk11util.c"
  [3] NSS_Shutdown(), line 292 in "nssinit.c"
  [4] nss_Init(configdir = 0x2b508 ".", certPrefix = 0xef675ea8 "", keyPrefix =
0xef675eac "", secmodName = 0xef675eb0 "secmod.db", readOnly = 1, nodb = 0),
line 237 in "nssinit.c"
  [5] NSS_Init(configdir = 0x2b508 "."), line 244 in "nssinit.c"
  [6] main(argc = 7, argv = 0xeffff78c), line 163 in "sdrtest.c"
(dbx) up  
Current function is SECMOD_Shutdown
  179       SECMOD_DestroyListLock(moduleLock);
(dbx) print moduleLock
moduleLock = (nil)
(dbx) list            
  179       SECMOD_DestroyListLock(moduleLock);
  180       moduleLock = NULL;
  181       /* free the internal module */
  182       SECMOD_DestroyModule(internalModule);
  183       internalModule = NULL;
  184       /* destroy the list */
  185       SECMOD_DestroyModuleList(modules);
  186       modules = NULL;
  187   
  188       /* make all the slots and the lists go away */
(dbx)

This particular crash is fixed by Bob's change to pk11util.c.

Bob, your changes to pk11util.c and pk11slot.c are fine.
You can check them in to fix the crash in our test suite.
I don't understand your change to pk11skey.c.  You might
want to have Terry or Nelson review it.

Comment 20

[Nelson Bolyard (seldom reads bugmail)]

SECU_ConfigDirectory makes a copy of the input arguments, obviating
the strdup.  And it handled the logic of defaulting to $HOME in the
absense of a -d option.  Most of our commands do this.  Should our
commands be consistent about this?

Comment 21

[Robert Relyea]

I probably requires looking at more context, so I'll put verbage here to help
whoever reviews it, BTW it's the change the is necessary if sdrtest is actually
ran with a real database. (Wah-Teh's traceback is a case where the database
wasn't available and NSS_Init call NSS_Shutdown before SECMOD_Init is called --
thus the null pointers for the lock.

The change is in the PK11_CleanKeyList() function. This function is only called
when you destroy a Slot (which only happens 1) at shutdown or 2) when you delete
a module). The KeyList is a cache of keys we build up as your system runs so
that we aren't pounding malloc all the time. Each slot has his own list of key
structures that get recycled. One of the expenses of creating a key is creating
a PKCS#11 session object with the key. The cache reuses these sessions as well.
Now when the key for is put on this list, the key's pointer to the slot is
release and the zero'ed. (PK11_FreeSymKey()). When the key is restored, that
slot pointer is restored (PK11_NewSymKey()). When we destroy a slot structure,
all the 'empty' key structures that have been cached in that slot are freed, and
the PKCS #11 sessions associated with them are closed. It's this close step that
was trying to dereference symKey->slot for a key on this list. Fortunately we
know what slot this session belongs to -- it's the slot we are trying to free,
so we use that pointer to tell the PKCS #11 code what slot and session to close.

bob

Comment 22

[Wan-Teh Chang]

OK, Bob, after reading your explanations a few times
I finally got it.  Patch reviewed.

Comment 23

[Marek Z. Jeziorek]

Mozilla 0.8 builds started today. We would consider taking reviewed low risk 
fixes if they are available today (Wednesday, 7/Feb/01) or tomorrow (Thursday, 
8/Feb/01). Otherwise, please set the target milestone to Mozilla 0.9. Thanks.

Comment 24

[Javier Delgadillo]

This will not make it into 0.8 since it would require picking up a new version
of NSS that has not been officially released yet.

The current implementation will not allow you to open 2 Security Module
databases on the Mac iff the user(s) switch profiles.  Embedding projects will
not care about this limitation unless they want to allow different users to use
different sets of smart cards on the Mac.



I

Comment 25

[Javier Delgadillo]

Setting target milestone to NSS 3.2

Comment 26

[Robert Relyea]

Javi, I'm going to close this bug now. NSS_Shutdown() seems to work in the
contexts that we currently call it. If you have any problems with PSM use then
open a new bug on that problem.

Comment 27

[Robert Relyea]

*** Bug 69470 has been marked as a duplicate of this bug. ***