Closed Bug 415167 Opened 16 years ago Closed 16 years ago

Memory leak in certutil


(NSS :: Tools, defect, P2)



(Not tracked)



(Reporter: julien.pierre, Assigned: julien.pierre)




(2 files)

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 .
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.
Assignee: julien.pierre.boogz → wtc
Component: Libraries → NSPR
Product: NSS → NSPR
QA Contact: libraries → nspr
Target Milestone: 3.12 → 4.7
Version: trunk → other
This leak was fixed by adding a PR_Cleanup call to certutil . Back to NSS.
Assignee: wtc → nobody
Component: NSPR → Tools
Product: NSPR → NSS
QA Contact: nspr → tools
Target Milestone: 4.7 → 3.12
Version: other → 3.11.9
Summary: Memory leak in NSS_Initialize → Memory leak in certutil
Assignee: nobody → julien.pierre.boogz
Attachment #300757 - Flags: review?(nelson)
Priority: -- → P2
Attachment #300757 - Flags: review?(nelson) → review+
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
Closed: 16 years ago
Resolution: --- → FIXED
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?
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.
Resolution: FIXED → ---
Attachment #321817 - Flags: review?
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.
Attachment #321817 - Flags: review? → review?(kengert)

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 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.
Attachment #321817 - Flags: superreview?(nelson)
Attachment #321817 - Flags: review?(kengert)
Attachment #321817 - Flags: review+
Blocks: 434808
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.
Attachment #321817 - Flags: superreview?(nelson) → superreview+
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
Julien, I'll leave it to you to close this bug as fixed again.
Target Milestone: 3.12 → 3.12.1
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.
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.