Beginning on October 25th, 2016, Persona will no longer be an option for authentication on BMO. For more details see Persona Deprecated.
Last Comment Bug 199082 - NSS_Shutdown fails for SSL servers
: NSS_Shutdown fails for SSL servers
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.7
: All All
: P1 normal (vote)
: 3.7.4
Assigned To: Nelson Bolyard (seldom reads bugmail)
: Bishakha Banerjee
: 181020 (view as bug list)
Depends on:
Blocks: 222065
  Show dependency treegraph
Reported: 2003-03-24 21:23 PST by Nelson Bolyard (seldom reads bugmail)
Modified: 2003-10-13 18:34 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

preliminary patch (5.46 KB, patch)
2003-03-24 21:27 PST, Nelson Bolyard (seldom reads bugmail)
rrelyea: review+
wtc: superreview+
Details | Diff | Splinter Review
Patch that's actually checked in (5.47 KB, patch)
2003-03-25 16:32 PST, Wan-Teh Chang
no flags Details | Diff | Splinter Review

Description Nelson Bolyard (seldom reads bugmail) 2003-03-24 21:23:38 PST
It has been reported that after a process acts as an SSL server, NSS_Shutdown
returns SECFailure, implying that a reference to some PKCS11 object has been leaked.

I investigated this claim and found several problems.

1. selfserv, NSS's SSL/TLS test server program does not check the result
returned by NSS_Shutdown.

2. NSS's SSL QA test scripts always kill the SSL server rather than shutting
it down gracefully.

3. SECMOD_Shutdown does not set the error code before returning SECFailure,
causing a "random" error code to be returned from NSS_Shutdown.

4. The pointers to the symmetric keys used to wrap master secrets in the server
session ID cache are stored in a static array in a function in ssl3con.c.  
They are never cleaned up (freed).

I have now developed a patch that addresses 3 of these 4 issues.  It 

a) changes selfserv to test the return value from NSS_Shutdown, and exit 
   with a non-zero exit code if NSS_Shutdown fails.

b) changes SECMOD_Shutdown to set the error code SEC_ERROR_BUSY before 
   returning SECFailure.

c) Adds a new function SSL_ShutdownServerSessionIDCache to ssl.h.  In its 
initial implementation, this function merely destroys the symmetric wrapping
keys used in the server session ID cache.  It should also completely shut
down the cache.

d) Changes selfserv to call SSL_ShutdownServerSessionIDCache before calling

This patch is not finished, but it may be sufficient to allow NSS-based SSL
servers to shutdown NSS without failure.  Whether it allows NSS to be 
initialized and shutdown repeatedly in the same SSL server process remains 
to be seen.  Patch forthcoming.
Comment 1 Nelson Bolyard (seldom reads bugmail) 2003-03-24 21:24:36 PST
Marking P1 for NSS 3.8.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2003-03-24 21:27:45 PST
Created attachment 118386 [details] [diff] [review]
preliminary patch

review invited
Comment 3 Wan-Teh Chang 2003-03-25 11:00:58 PST
Comment on attachment 118386 [details] [diff] [review]
preliminary patch

This patch is good. I've asked Sailesh and Javi to test it.
The version number of the new function needs to be changed
to NSS_3.7.4 before we check it in.
Comment 4 Wan-Teh Chang 2003-03-25 16:32:36 PST
Created attachment 118503 [details] [diff] [review]
Patch that's actually checked in

The only difference from Nelson's patch is that the version
of the new symbol in ssl.def says NSS_3.7.4 instead of NSS_3.8.
Comment 5 Wan-Teh Chang 2003-03-25 16:33:13 PST
Patch checked into the TIP (3.8) and NSS_3_7_BRANCH (3.7.4).
Comment 6 Wan-Teh Chang 2003-03-26 07:59:05 PST
Comment on attachment 118386 [details] [diff] [review]
preliminary patch


Could you make the following fixes to lib/ssl/sslsnce.c on the trunk?

1. Declare SSL_ShutdownServerSessionIDCacheInstance with 'static'
and add a return statement?  Both Bob and I missed this :-)

>+SSL_ShutdownServerSessionIDCacheInstance(cacheDesc *cache)
>+    /* if single process, close down, clean up.
>+    ** if multi-process, TBD.
>+    */

2. SSL_ShutdownServerSessionIDCache should check the return value
of SSL3_ShutdownServerCache().

>+    SSL3_ShutdownServerCache();
>+    return SSL_ShutdownServerSessionIDCacheInstance(&globalCache);
Comment 7 Nelson Bolyard (seldom reads bugmail) 2003-03-26 13:26:04 PST
In response to the above review comments:

1. I agree that SSL_ShutdownServerSessionIDCacheInstance should return a value.

2. I don't agree that SSL_ShutdownServerSessionIDCacheInstance should be static.  

The intent is that this function is callable from elsewhere in libSSL.  
The intent is that libSSL will move away from having a single server session
that is share by all server threads/sockets in the process towarrds having
multiple server session caches, potentially server cache instance per logical
server in the process.  There are already functions for initializing and
inheriting individual cache instances (as well as the older functions for
initializing and inheriting the single global instance), and those functions 
that deal with individual instances are not static.  So, now that I'm adding
functions to shutdown the cache, there should be (and are) functions both to
shut down individual instances and the global instance.  Full support for
individual instances is not ready yet, but I think the code is moving in the
right direction.  Eventually, the functions that implicitly access the one
global instance should fall into disuse, and the functions that manipulate
an explicit instance should be the ones that remain in use.

3, SSL_ShutdownServerSessionIDCache could check the return value of
SSL3_ShutdownServerCache(), but regardless of that return value, it should
continue to shutdown the global instance.  So, I'm not sure what the point
of the check would be.  Maybe SSL3_ShutdownServerCache should return void (?).
Comment 8 Nelson Bolyard (seldom reads bugmail) 2003-05-08 17:48:16 PDT
*** Bug 181020 has been marked as a duplicate of this bug. ***
Comment 9 Nelson Bolyard (seldom reads bugmail) 2003-10-13 18:34:17 PDT
Somehow, the "preliminary patch" above was checked in and this bug got marked
RESOLVED/FIXED, even though the patch was not finished.  The new code doesn't 
free the cache memory.  It leaks a LOT of memory.  But it fell off the radar.

I have filed bug 222065 to track this leak.  

Note You need to log in before you can comment on or make changes to this bug.