Bug 769616

Summary: CKFW leaks memory when loading libnsspem.so after upgrade of nss
Product: Red Hat Enterprise Linux 6 Reporter: Kamil Dudka <kdudka>
Component: nssAssignee: Elio Maldonado Batiz <emaldona>
Status: CLOSED ERRATA QA Contact: Aleš Mareček <amarecek>
Severity: medium Docs Contact:
Priority: high    
Version: 6.2CC: amarecek, jakub, jrieden, kengert, rrelyea, syeghiay
Target Milestone: rcKeywords: Regression, Reopened
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: nss-3.13.3-3.el6 Doc Type: Bug Fix
Doc Text:
No documentation needed
Story Points: ---
Clone Of: Environment:
Last Closed: 2012-06-20 07:23:36 UTC Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Attachments:
Description Flags
a test-case
none
proposed fix rrelyea: review+

Description Kamil Dudka 2011-12-21 13:29:39 UTC
Version-Release number of selected component (if applicable):
nss-3.12.10-17.el6_2


How reproducible:
100%


Steps to Reproduce:
1. run the attached test-case

  
Actual results:
43,008 bytes in 256 blocks are definitely lost in loss record 75 of 75
   at 0x4A04A28: calloc (vg_replace_malloc.c:467)
   by 0x331A4236CC: PR_NewLock (ptsynch.c:174)
   by 0x4C36D24: nssCKFWMutex_Create (mutex.c:137)
   by 0x4C36989: nssCKFWInstance_Create (instance.c:245)
   by 0x4C40FAF: NSSCKFWC_Initialize (wrap.c:205)
   by 0x4E98F06: secmod_ModuleInit (pk11load.c:252)
   by 0x4E99886: secmod_LoadPKCS11Module (pk11load.c:492)
   by 0x4EAE354: SECMOD_LoadModule (pk11pars.c:1121)
   by 0x4EAE5C7: SECMOD_LoadUserModule (pk11pars.c:1209)
   by 0x40080F: main (ckfw-leak.c:19)

LEAK SUMMARY:
   definitely lost: 43,008 bytes in 256 blocks
   indirectly lost: 0 bytes in 0 blocks
     possibly lost: 0 bytes in 0 blocks
   still reachable: 11,137 bytes in 88 blocks
        suppressed: 0 bytes in 0 blocks


Expected results (I get this after downgrade to nss-3.12.10-11.el6):
LEAK SUMMARY:
   definitely lost: 0 bytes in 0 blocks
   indirectly lost: 0 bytes in 0 blocks
     possibly lost: 0 bytes in 0 blocks
   still reachable: 11,137 bytes in 88 blocks
        suppressed: 0 bytes in 0 blocks


Additional info:
The CKFW code has been broken since RHEL-6.0, but it did not take any effect until nss-3.12.10-13.el6, where the following patch was applied:

--- ./mozilla/security/nss/lib/ckfw/pem/pinst.c.735047
+++ ./mozilla/security/nss/lib/ckfw/pem/pinst.c
@@ -377,7 +377,6 @@ AddCertificate(char *certfile, char *key
                CK_SLOT_ID slotID)
 {
     pemInternalObject *o;
-    SECItem certDER;
     CK_RV error = 0;
     int objid, i;
     int nobjs = 0;
@@ -385,7 +384,6 @@ AddCertificate(char *certfile, char *key
     char *ivstring = NULL;
     int cipher;

-    certDER.data = NULL;
     nobjs = ReadDERFromFile(&objs, certfile, PR_TRUE, &cipher, &ivstring, PR_TRUE /* certs only */);
     if (nobjs <= 0) {
         nss_ZFreeIf(objs);
@@ -469,32 +467,36 @@ pem_Initialize
 )
 {
     CK_RV rv;
-    /* parse the initialization string and initialize CRLInstances */
+    /* parse the initialization string */
     char **certstrings = NULL;
+    char *modparms = NULL;
     PRInt32 numcerts = 0;
     PRBool status, error = PR_FALSE;
     int i;
+    CK_C_INITIALIZE_ARGS_PTR modArgs = NULL;
+
+    if (!fwInstance) return CKR_ARGUMENTS_BAD;
+
+    modArgs = NSSCKFWInstance_GetInitArgs(fwInstance);
+    if (modArgs &&
+       ((modArgs->flags & CKF_OS_LOCKING_OK) || (modArgs->CreateMutex != 0))) {
+        return CKR_CANT_LOCK;
+    }

Comment 1 Kamil Dudka 2011-12-21 13:31:42 UTC
Created attachment 549024 [details]
a test-case

Comment 3 Kamil Dudka 2011-12-21 15:47:59 UTC
Created attachment 549053 [details]
proposed fix

Comment 4 Bob Relyea 2012-01-03 19:16:36 UTC
Comment on attachment 549053 [details]
proposed fix

r+

So the mutex is an instance variable, do we do the mutex destroy in the finalize as well? (Sometimes the framework depends too much on arenas to do cleanup).

bob

Comment 5 Kamil Dudka 2012-01-03 20:52:01 UTC
(In reply to comment #4)
> So the mutex is an instance variable, do we do the mutex destroy in the
> finalize as well?

If finalize means NSSCKFWC_Finalize(), it seems we do.  It calls nssCKFWInstance_Destroy(), which calls nssCKFWMutex_Destroy(fwInstance->mutex).

Comment 6 Bob Relyea 2012-01-03 21:13:50 UTC
Cool, then it was just the error path that was wrong.

Elio, could you make sure we get an upstream bug for this. The issue affects all ckfw modules, not just libpem... most particularly the builtins module (but also libcapi on windows and libmkey on mac).

bob

Comment 7 Elio Maldonado Batiz 2012-01-03 22:29:05 UTC
(In reply to comment #6)
Done, at https://bugzilla.mozilla.org/show_bug.cgi?id=714961

Comment 10 Elio Maldonado Batiz 2012-01-13 21:26:42 UTC
Patch approved and committed upstream 
https://bugzilla.mozilla.org/show_bug.cgi?id=714961#c4

Comment 12 Kai Engert (:kaie) (inactive account) 2012-03-05 18:11:41 UTC
Kamil, as I understand it, your patch was added upstream and will be contained in NSS 3.13.2 and newer.

I understand Elio already works on including the newer 3.13.3 into RHEL-6.3

I conclude this bug can be closed?

Comment 21 Elio Maldonado Batiz 2012-04-26 18:27:29 UTC
    Technical note added. If any revisions are required, please edit the "Technical Notes" field
    accordingly. All revisions will be proofread by the Engineering Content Services team.
    
    New Contents:
No documentation needed

Comment 22 errata-xmlrpc 2012-06-20 07:23:36 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

http://rhn.redhat.com/errata/RHSA-2012-0973.html