205948 - Backport leak fixes to NSS_3_3_BRANCH

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Kirk Erickson]

Lily Hsiao ran purify on NSS version is 3.3 and NSPR is 4.1.3, on solaris,
and yielded 15 potential memory leaks. I investigated the first, and
found that the TIP had a change which addressed that leak.  I decided
to document all the MLK's and come up with a complete patch.

The complete list is in the first attachment.
Each MLK is numbered for reference.

Separate bugs will be opened for leaks still present at the TIP;
this bug is specifically for the collection which can be backported
from releases after 3.3.

Comment 1

[Kirk Erickson]

Attachment: Lily's (MLK) purify output

Complete list of all 15 potential leaks from Lily Hsiao.
NSS version is 3.3 and NSPR is 4.1.3, on solaris.

Comment 2

[Kirk Erickson]

Attachment: Address MLK#1

This patch addresses MLK#1 only.

PK11_ReadMechanismList() is allocating
slot->mechanismList.  It frees on the way in if its been allocated
previously (called before on the same slot).  There is an additional
free at the TIP in PK11_DestroySlot(), which this patch backports to 
NSS_3_3_BRANCH.

Comment 3

[Kirk Erickson]

Set P2, Status Whiteboard 3.3.5.

Comment 4

[Kirk Erickson]

Regarding MLK#2:

In pk11_NewSession(): mozilla/security/nss/lib/softoken/pkcs11u.c

#ifdef PKCS11_USE_THREADS
    session->objectLock = PZ_NewLock(nssILockObject);
    if (session->objectLock == NULL) {
        PORT_Free(session);
        return NULL;
    }
#else
    session->objectLock = NULL;
#endif

Where are we freeing session->objectLock?
In pk11_DestroySession().  Its qualified:

PK11_USE_THREADS(PZ_DestroyLock(session->objectLock);)

NSS_3_3_BRANCH is identical with the TIP, and looks correct.
Apparently, Purify is fooled by the macros.  There's no fix
to backport in any case.

Note, pk11_NewSession is using PKCS11_USE_THREADS,
and pk11_DestroySession is using PK11_USE_THREADS.
PK11_USE_THREADS is a noop:

PKCS11_USE_THREADS is always defined.
So PK11_USE_THREADS is always a noop:

#ifdef PKCS11_USE_THREADS
#define PK11_USE_THREADS(x) x
#else
#define PK11_USE_THREADS(x)
#endif

So MLK#2 doesn't contribute to our patch.

Comment 5

[Kirk Erickson]

MLK#3 is the same as MLK#2.  Nothing to backport, no TIP bug.

Regarding MLK#4: 
There's but one PORT_Alloc call in pk11_NewSession(), its
for the session structure.  This is freed unconditionally
by pk11_DestroySession().  

pk11_DestroySession() is called (when refCount is 0).  There's a single
call from pk11_FreeSession() in 33.  This matches the TIP.  The
pk11_FreeSession() is the same on NSS_3_3_BRANCH as the tip.

pk11_FreeSession() is called from 85 places on NSS_3_3_BRANCH.
pk11_FreeSession() is called from 83 places at the TIP.

Search of bugzilla database for "leak" yielded alot of bugs:
Created /home/ke119340/nss/205948/leaks-bugzilla.html
	There were 18 bugs NEW and ASSIGNED.
Created /home/ke119340/nss/205948/leaks-bugzilla-resolved.html
	There were 63 bugs RESOLVED.
I combed the RESOLVED list for anything related, and came up empty
handed.  The fix for 135818 (Internal slot reference leaks in
lib/pk11wrap/pk11slot.c) was apparently already backported.
The fix for 195914 is present too.

Since pk11_FreeSession() is the same at the TIP and NSS_3_3_BRANCH,
and there are as many or more calls to it total, and I was unable
to identify a related bug - I yielded nothing relevant to MLK#4
to backport.

Comment 6

[Kirk Erickson]

Attachment: Address MLK#1 and MLK#5

Regarding MLK#5:

CERT_OpenCertDB doesn't exist at the TIP. nsslowcert_OpenCertDB()
in softoken opens now.	Both create do:

    handle->dbMon = PZ_NewMonitor(nssILockCertDB);

At the TIP, nsslowcert_ClosePermCertDB() destroys the monitor.
Created patch to do the same in __CERT_ClosePermCertDB at 33:

    if (handle->dbMon) {
	PZ_DestroyMonitor(handle->dbMon);
	handle->dbMon = NULL;
    }

Comment 7

[Kirk Erickson]

Regarding MLK#7:

PR_CreatePR_CreateThread() lives in
./mozilla/nsprpub/pr/src/misc/prtpool.c.
It calls a static alloc_threadpool() which does:

    tp = (PRThreadPool *) PR_CALLOC(sizeof(*tp));

This is the only PR_Calloc call in the file.
The static delete_threadpool() frees a thread pool.

./mozilla/nsprpub/pr/src/misc/prtpool.c:569:delete_threadpool(PRThreadPool *tp)
./mozilla/nsprpub/pr/src/misc/prtpool.c:646:	delete_threadpool(tp);
./mozilla/nsprpub/pr/src/misc/prtpool.c:1212:	delete_threadpool(tpool);

Its called from alloc_threadpool(646) on initialization failure
(646), and from PR_JoinThreadPool(1212), which waits for
termination of worker threads and reclaims threadpool resources.

It appears that Lily's app is calling in initializing via
SSOTokenService().  This is not something for backport to
NSS_3_3_BRANCH; not relevant to this bug.

Its not clear to me if Lily called PR_ShutdownThreadPool() the
storage would be freed.	 I emailed Wan-Teh, copying Lily with
these notes to enable Lily to address MLK#7.

Comment 8

[Kirk Erickson]

Attachment: Address MLK#1, MLK#5, MLK#10, and MLK#11

MLK#8 and MLK#9 are the same as MLK#7.
MLK#10 is the same as MLK#5.

Regarding MLK#11:

nss_OpenKeyDB() is freeing:

    name = PR_smprintf("%s" PATH_SEPARATOR "%s",configdir,prefix);
    if (name == NULL)
    return SECFailure;
    keydb = SECKEY_OpenKeyDB(readOnly, nss_keydb_name_cb, (void *)name);
    if (keydb == NULL)
    return SECFailure;
    SECKEY_SetDefaultKeyDB(keydb);
    PORT_Free(name);

except when SECKEY_OpenKeyDB() fails.  Updated combined patch
to cover that case.

Note, nss_OpenKeyDB() doesn't exist at the TIP.  The counterpart
nsslowkey_OpenKeyDB() uses PORT_Strdup(), and deallocates
properly in the error path.  So there's no bug to file against
the TIP.

Comment 9

[Kirk Erickson]

Regarding MLK#12:

	__hash_open	   [libnss3.so]
	SEC_OpenPermCertDB [libnss3.so]

NSS_3_3_BRANCH deallocates 'certdbname' properly.  'hashInfo' and
'versionEntry' are static.

There's nothing to fix in SEC_OpenPermCertDB().  This might have
been something that was fixed on the NSS_3_3_BRANCH subsequent to
the version Lily engaged (3.3).

Regarding MLK#13:

	strdup	   [libc.so.1]
	SECKEY_OpenKeyDB [libnss3.so]

The only strdup in SECKEY_OpenKeyDB is:

	handle->dbname = PORT_Strdup(dbname);

All returns deallocate this data structure properly.
Yielded nothing to add to our combined patch.

MLK#14 is identical to MLK#12.

Regarding MLK#15:

	PR_Calloc	   [prmem.c:64]
	bl_LoadLibrary [loader.c]
	freebl_LoadDSO [loader.c]

The only candidate data structure is allocated thusly:

	lib = PR_NEWZAP(BLLibrary);

Its a static, which the function returns.  Its deallocated if
there's a failure loading the library or by bl_UnloadLibrary()
subsequently.  I guess Purify is fooled somehow in the 
successful load path.  We had UMR's for freebl_LoadDSO() that
apparently fooled Purify too.

This completes a pass over all 15 of Lily's leaks.

Comment 10

[Kirk Erickson]

CC'd Ian.  Would like him to review the patch.

Comment 11

[Ian McGreer]

Comment on attachment 123646 [details] [diff] [review]
Address MLK#1, MLK#5, MLK#10, and MLK#11

looks good.

Comment 12

[Kirk Erickson]

Comment on attachment 123646 [details] [diff] [review]
Address MLK#1, MLK#5, MLK#10, and MLK#11

Checked into NSS_3_3_BRANCH:

Checking in mozilla/security/nss/lib/certdb/pcertdb.c;
/cvsroot/mozilla/security/nss/lib/certdb/Attic/pcertdb.c,v  <--  pcertdb.c
new revision: 1.16.2.5; previous revision: 1.16.2.4
done
Checking in mozilla/security/nss/lib/nss/nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--	nssinit.c
new revision: 1.21.2.2; previous revision: 1.21.2.1
done
Checking in mozilla/security/nss/lib/pk11wrap/pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.14.2.7; previous revision: 1.14.2.6
done

Comment 13

[Wan-Teh Chang]

Kirk, Ian, has this bug been fixed?

Comment 14

[Ian McGreer]

Yup.