222065 - SSL_ShutdownServerSessionIDCache leaks the cache memory

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Nelson Bolyard (seldom reads bugmail)]

SSL_ShutdownServerSessionIDCache does not free the process's copy of the 
(possibly shared) memory that contains the cache data.  Consequently, all
that memory is leaked.  

SSL_ShutdownServerSessionIDCache was created as part of the resolution of
bug 199082.  A "preliminary" version of the patch was checked in, but the
work was not complete.  Then the bug was marked resolved/fixed.  Then it
fell off the radar.  No bug was created to track the fact that the work 
was undone, until now.

Comment 1

[Nelson Bolyard (seldom reads bugmail)]

Marking P1 for NSS 3.9

Comment 2

[Wan-Teh Chang]

We need to put the fix on the 3.8 branch as well.  Setting target
milestone 3.8.3.

Comment 3

[Nelson Bolyard (seldom reads bugmail)]

Attachment: preliminaty patch v1

This patch appears to be correct for single-process servers.  
I believe it is no worse than before for multi-process servers.
There is some code specifically for WINNT builds (using the NT version of NSPR)

and I have not yet tested that code.  I will do so before checking anything in.


I have also found a large number of memleaks whose allocation stacks all have
this in common:

		    calloc	   [MSVCRT.DLL]
		    PR_Calloc	   [prmem.c:484]
		    PR_NewLock	   [prulock.c:194]
		    secmodCreateMutext [pk11load.c:61]
    
    /* build the PKCS #11 2.01 lock files */
    CK_RV PR_CALLBACK secmodCreateMutext(CK_VOID_PTR_PTR pmutex) {
 =>	*pmutex = (CK_VOID_PTR) PZ_NewLock(nssILockOther);
	if ( *pmutex ) return CKR_OK;
	return CKR_HOST_MEMORY;
    }

It appears these locks are never freed.

Comment 4

[Wan-Teh Chang]

Comment on attachment 133538 [details] [diff] [review]
preliminaty patch v1

This patch is basically good, but it needs work.

1. The most serious problem is the #ifdef WINNT code
that calls sslMutex_2LevelDestroy.  That piece of
code should be deleted because the subsequent
sslMutex_Destroy calls will call sslMutex_2LevelDestroy
on WINNT.

Please add a comment to explain why the
sslMutex_Destroy calls are only made if
cache->sharedCache->everInherited is false.
The reason is not obvious at all.

2. In SSL_InheritMPServerSIDCacheInstance, we
should do 

>+    cache->sharedCache->everInherited = PR_TRUE;

right before we set isMultiProcess to PR_TRUE
and return SECSuccess.	At least, there must not
be any "goto loser" statement after we set
cache->sharedCache->everInherited to PR_TRUE.

Please add a comment to explain the purpose of the
local variable "my".

Comment 5

[Nelson Bolyard (seldom reads bugmail)]

Attachment: patch v2 - addressing Wan-Teh's comments

Please also review the patch for bug 222726, which is related to this bug.

Comment 6

[Wan-Teh Chang]

Comment on attachment 133604 [details] [diff] [review]
patch v2 - addressing Wan-Teh's comments

I verified that all the changes I suggested have been made.
r=wtc.

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Fix checked in on both trunk and NSS 3.8 branch.

Comment 8

[Wan-Teh Chang]

Attachment: Supplemental patch

I reviewed Nelson's patch again and found a bug in a piece
of code that is only used by the WINNT build for servers.
We should not set cache->sidCacheLocks to newLocks before
the memcpy call, otherwise the memcpy call will copy the
newLocks array onto itself.

This supplemental patch fixes that bug.

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 133722 [details] [diff] [review]
Supplemental patch

r=MisterSSL

Comment 10

[Wan-Teh Chang]

The supplemental patch has been checked in on both trunk
and NSS 3.8 branch.