Memory leak in certutil

RESOLVED FIXED in 3.12.1

Status

NSS
Tools
P2
normal
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

3.11.9
3.12.1
x86
SunOS

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

473 bytes, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Details | Diff | Splinter Review
707 bytes, patch
kaie
: review+
Nelson Bolyard (seldom reads bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

9 years ago
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 .
(Assignee)

Comment 1

9 years ago
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
(Assignee)

Comment 2

9 years ago
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
(Assignee)

Updated

9 years ago
Summary: Memory leak in NSS_Initialize → Memory leak in certutil
(Assignee)

Comment 3

9 years ago
Created attachment 300757 [details] [diff] [review]
Add PR_Cleanup call before exiting
Assignee: nobody → julien.pierre.boogz
Status: NEW → ASSIGNED
Attachment #300757 - Flags: review?(nelson)
(Assignee)

Updated

9 years ago
Priority: -- → P2
Attachment #300757 - Flags: review?(nelson) → review+
(Assignee)

Comment 4

9 years ago
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
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 5

9 years ago
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?
(Assignee)

Comment 6

9 years ago
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 → ---
(Assignee)

Comment 7

9 years ago
Created attachment 321817 [details] [diff] [review]
move PR_Cleanup to main
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.
(Assignee)

Updated

9 years ago
Attachment #321817 - Flags: review? → review?(kengert)
(Assignee)

Comment 9

9 years ago
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 10

9 years ago
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.
Attachment #321817 - Flags: superreview?(nelson)
Attachment #321817 - Flags: review?(kengert)
Attachment #321817 - Flags: review+

Updated

9 years ago
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+

Comment 12

9 years ago
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

Comment 13

9 years ago
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.
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.