Closed Bug 415167 Opened 13 years ago Closed 13 years ago
Memory leak in certutil
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 Address ========== ====== =========== ======================================= 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
Status: NEW → ASSIGNED
Attachment #300757 - Flags: review?(nelson)
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 done
Status: ASSIGNED → RESOLVED
Closed: 13 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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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)
Nelson, 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 r=kengert 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 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 done
Julien, I'll leave it to you to close this bug as fixed again.
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.
Status: REOPENED → RESOLVED
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.