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 415167 - Memory leak in certutil
: Memory leak in certutil
Product: NSS
Classification: Components
Component: Tools (show other bugs)
: 3.11.9
: x86 SunOS
: P2 normal (vote)
: 3.12.1
Assigned To: Julien Pierre
Depends on:
Blocks: 434808
  Show dependency treegraph
Reported: 2008-01-31 15:22 PST by Julien Pierre
Modified: 2008-08-14 03:00 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---

Add PR_Cleanup call before exiting (473 bytes, patch)
2008-01-31 15:35 PST, Julien Pierre
nelson: review+
Details | Diff | Splinter Review
move PR_Cleanup to main (707 bytes, patch)
2008-05-20 13:20 PDT, Julien Pierre
kaie: review+
nelson: superreview+
Details | Diff | Splinter Review

Description Julien Pierre 2008-01-31 15:22:00 PST
I built the trunk on solaris. I created an empty DB with certutil.
Then I ran certutil -d . -L under dbx checkleaks (check -leaks -frames 16 -leaks 16) .

Here is the output :

Actual leaks report    (actual leaks:            1  total size:         72 bytes)

  Total     Num of  Leaked     Allocation call stack
  Size      Blocks  Block
==========  ====== =========== =======================================
        72       1  0x80bdbe8  calloc < PR_Calloc < error_get_my_stack < nss_ClearErrorStack < NSSArena_Create < NSSTrustDomain_Create < STAN_LoadDefaultNSS3TrustDomain < nss_Init < NSS_Initialize < certutil_main < main

I didn't see that stack in our ignore file. I'm wondering why. 

This bug doesn't occur on the NSS_3_11_BRANCH .
Comment 1 Julien Pierre 2008-01-31 15:31:48 PST
It turns out the bug is not in NSS bug NSPR . I reproduced the problem with NSS 3.11.9 against NSPR 4.7, which we are about to release.
Comment 2 Julien Pierre 2008-01-31 15:34:29 PST
This leak was fixed by adding a PR_Cleanup call to certutil . Back to NSS.
Comment 3 Julien Pierre 2008-01-31 15:35:54 PST
Created attachment 300757 [details] [diff] [review]
Add PR_Cleanup call before exiting
Comment 4 Julien Pierre 2008-02-01 14:05:45 PST
Thanks for the review, Nelson. I checked this in to the trunk.

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.123; previous revision: 1.122
Comment 5 Kai Engert (:kaie) 2008-05-20 11:35:13 PDT
It seems this patch caused the regression reported in bug 434808.

While there have been many changes to certutil in the regression window, the deadlock happens on the line that is being added in this bug.

I backed out this line, and the deadlock is gone.

Does this cleanup not work well with the recursive nature of certutil's batch processing?
Comment 6 Julien Pierre 2008-05-20 11:55:47 PDT
You are right. The PR_Cleanup should be in main, not certutil_main .
I wasn't aware that anyone was actually using the batch function I added years ago to certutil. I will fix it.
Comment 7 Julien Pierre 2008-05-20 13:20:07 PDT
Created attachment 321817 [details] [diff] [review]
move PR_Cleanup to main
Comment 8 Nelson Bolyard (seldom reads bugmail) 2008-05-20 13:36:02 PDT
Comment on attachment 321817 [details] [diff] [review]
move PR_Cleanup to main

This review request is not directed to any particular reviewer.

Julien, is it possible for certutil_main to return without having
initialized NSPR?  I think we wouldn't want to have the first call
to NSPR be PR_Cleanup.
Comment 9 Julien Pierre 2008-05-20 13:43:07 PDT

I could swear that I had typed in Kai's email for the reviewer. I must have made a typo, and not been warned about it.

About your remark, it's possible but very unlikely that NSPR was not initialized and certutil_main returned. Even so, it's legal for PR_Cleanup to be the first NSPR call. It will cause NSPR to be immediately initialized and then cleaned up.
Comment 10 Kai Engert (:kaie) 2008-05-21 04:41:14 PDT
Comment on attachment 321817 [details] [diff] [review]
move PR_Cleanup to main


I think this would be a safe patch to land on the 3.12 release branch as well, because it's zero risk and restricted to certutil.

Requesting second review.
Comment 11 Nelson Bolyard (seldom reads bugmail) 2008-05-28 16:06:12 PDT
Comment on attachment 321817 [details] [diff] [review]
move PR_Cleanup to main

I think there is an NSPR function that answers the question: is NSPR initialized.  I might prefer to see this code only call PR_Cleanup if it had already been initialized.  But that's not absolutely necessary.
Comment 12 Kai Engert (:kaie) 2008-05-29 10:18:15 PDT
I think I've missed the opportunity to get this fixed in 3.12.0

Let me check in Julien's patch now (to trunk for 3.12.1), and if he wants to address Nelson's optional proposal, this can be done in a separate patch.

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.138; previous revision: 1.137
Comment 13 Kai Engert (:kaie) 2008-05-29 10:21:43 PDT
Julien, I'll leave it to you to close this bug as fixed again.
Comment 14 Nelson Bolyard (seldom reads bugmail) 2008-08-14 03:00:15 PDT
80 days later ...
I'm marking this bug fixed. 
If someone thinks it should remain open, please file a new bug about 
whatever changes remain to be made.

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