415167 - Memory leak in certutil

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Julien Pierre]

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 .

Comment 1

[Julien Pierre]

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]

This leak was fixed by adding a PR_Cleanup call to certutil . Back to NSS.

Comment 3

[Julien Pierre]

Attachment: Add PR_Cleanup call before exiting

Comment 4

[Julien Pierre]

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

Comment 5

[Kai Engert [:KaiE:]]

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]

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]

Attachment: move PR_Cleanup to main

Comment 8

[Nelson Bolyard (seldom reads bugmail)]

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]

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

[Kai Engert [:KaiE:]]

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 11

[Nelson Bolyard (seldom reads bugmail)]

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:]]

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

[Kai Engert [:KaiE:]]

Julien, I'll leave it to you to close this bug as fixed again.

Comment 14

[Nelson Bolyard (seldom reads bugmail)]

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.