PKIX_Shutdown is not called by nssinit.c

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P1
trivial
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: dbaron, Assigned: Alexei Volkov)

Tracking

({mlk})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: PKIX)

Attachments

(1 attachment, 2 obsolete attachments)

6.35 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
This bug is filed based on the NSS_3_12_ALPHA1B tag, which is what is currently used by the trunk of Firefox, etc.

security/nss/lib/nss/nssinit.c currently calls PKIX_Initialize but not PKIX_Shutdown.  It seems that someplace in NSS_Shutdown should have the following code:

+    if (plContext) {
+        PKIX_Shutdown(plContext);
+        plContext = NULL;
+    }

This alone doesn't fix any memory leaks, but with improvements to PKIX_Shutdown I think it would fix some memory leaks.
Assignee: nobody → alexei.volkov.bugs
OS: Windows XP → All
Priority: -- → P2
Hardware: PC → All
Whiteboard: PKIX
Target Milestone: --- → 3.12
Version: 3.12 → trunk
Depends on: 391815
The fix for this bug should not be committed while it remains blocked by 
bug 391815, and bug 391815 remains unresolved.
(Assignee)

Comment 2

10 years ago
nss shutdown function now calls PKIX_Shutdown. The fix was committed with the patch for bug 395427. But still, PKIX_Shutdown is ifdef-ed awaiting a fix for bug  391815. So, will keep this bug open until ifdef for PKIX_Shutdown call is removed.   
(Assignee)

Updated

10 years ago
Priority: P2 → P1
(Assignee)

Comment 3

10 years ago
Created attachment 281745 [details] [diff] [review]
remove ifdef for PKIX_Shutdown
Attachment #281745 - Flags: review?(nelson)
Comment on attachment 281745 [details] [diff] [review]
remove ifdef for PKIX_Shutdown

This patch is obviously correct, as far as it goes, but it also needs to remove from the file of ignored leak stacks any stacks that will be fixed by the removal of the ifdef.  It may or may not be possible to do that now, depending on whether all the leak stacks caused by PKIX initialization have been put into the ignored file already, or not.  They can't be removed until they are put in, of course.  
The next patch should have two parts:
1) the part for nssinit.c that removes the ifdefs (the present patch), and 
2) the patch to the "ignored" file.
Attachment #281745 - Flags: review?(nelson) → review-

Comment 5

10 years ago
This leak is already detected by our tools, dependency on bug 391815 can be
removed.
Comment on attachment 281745 [details] [diff] [review]
remove ifdef for PKIX_Shutdown

Please attach a patch that will remove the leak stacks that will be fixed by this patch.  I will give this patch and that one r+ at the same time.
Attachment #281745 - Attachment description: remove ifdev for PKIX_Shutdown → remove ifdef for PKIX_Shutdown
(Assignee)

Comment 7

10 years ago
Created attachment 283055 [details] [diff] [review]
ignore file patch

PKIX_Shutdown will be called from NSS_Shutdown with patch in attachment 281745 [details] [diff] [review] applied to the trunk.

This patch removes all one time initialization leaks from ignore file in the assumption that PKIX_Shutdown will free these memories blocks. But memory checking tinderbox should go orange, since PKIX_Shitdown currently does not take care of at least one real leak: 

  -*/main/NSS_Initialize/*/PKIX_Initialize/PKIX_PL_Initialize/PR_NewLock/**
Attachment #283055 - Flags: review?(nelson)
Comment on attachment 283055 [details] [diff] [review]
ignore file patch

Please do not remove any stacks that you know are not actually fixed by the first patch for this bug.
Attachment #283055 - Flags: review?(nelson) → review-

Comment 9

10 years ago
Created attachment 283353 [details] [diff] [review]
Patch v1.

I tested memory leaks with PKIX shutdown uncommented to find leaks which stays there. There was only one PKIX leak found (Alexei already mentioned it in comment 7), so I opened new bug for this (bug 398401) and reassigned stacks related to this bug to it in ignore list. 

Now we can uncomment PKIX shutdown + remove all stacks assigned to it from ignore list and close this bug. I prepared patch for this (replacing previous patches).
Attachment #281745 - Attachment is obsolete: true
Attachment #283055 - Attachment is obsolete: true
Attachment #283353 - Flags: review?(nelson)
Comment on attachment 283353 [details] [diff] [review]
Patch v1.

r=nelson
Alexei or Slavo, please commit this patch ASAP.
Attachment #283353 - Flags: review?(nelson) → review+
(Assignee)

Comment 11

10 years ago
/cvsroot/mozilla/security/nss/tests/memleak/ignored,v  <--  ignored
new revision: 1.27; previous revision: 1.26
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.84; previous revision: 1.83
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.