Closed Bug 335036 Opened 15 years ago Closed 15 years ago

NSS_Shutdown() does not check that NSS is initialized


(NSS :: Libraries, defect, P2)



(Not tracked)



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



(2 files, 1 obsolete file)

I was working on a race condition and added the creation of a lock in NSS_Initialize, and its destruction in NSS_Shutdown().
I ran . One of the tests in tries to create a new DB in a nonexisting directory. NSS_Initialize does not complete , and thus my new lock was not created. But NSS_Shutdown() is called, and proceeds through trying to shutdown many things that were never started in the first place. I think that's a bug. NSS_Shutdown() should check the variable nss_IsInitted and should not proceed if it's already PR_FALSE .
Assignee: nobody → julien.pierre.bugs
Attachment #219396 - Flags: review?(nelson)
Priority: -- → P2
Comment on attachment 219396 [details] [diff] [review]
fail with SEC_ERROR_NOT_INITIALIZED if there is no NSS to shut down

besides the new symbol "race", this patch doesn't define a string for the new error code. r-
Attachment #219396 - Flags: review?(nelson) → review-
Also define a string for SEC_ERROR_CRL_ALREADY_EXISTS, which didn't have one yet.
Attachment #219396 - Attachment is obsolete: true
Attachment #222075 - Flags: superreview?(alexei.volkov.bugs)
Attachment #222075 - Flags: review?(nelson)
Comment on attachment 222075 [details] [diff] [review]
incorporate Nelson's feedback

Attachment #222075 - Flags: review?(nelson) → review+
Attachment #222075 - Flags: superreview?(alexei.volkov.bugs) → superreview+
Thanks for the quick reviews. I checked this in to the tip :

Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision: 1.73; previous revision: 1.72

And to NSS_3_11_BRANCH :

Checking in nssinit.c;
/cvsroot/mozilla/security/nss/lib/nss/nssinit.c,v  <--  nssinit.c
new revision:; previous revision:

The new error code was added as part of the checkin to the fix for bug 337789 .
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11.2
One of the tests in, "certutil in a nonexisting directory", now fails because of this change. The script checks for a return code of 255, but certutil returns 1.

This is because of the following code in certutil :

    if ((initialize == PR_TRUE) && NSS_Shutdown() != SECSuccess) {

    if (rv == SECSuccess) {
	return 0;
    } else {
	return 255;

Is there any reason to distinguish an NSS_Shutdown() failure from other failures with a return status of 1 ?
Resolution: FIXED → ---
Statements like:

    if ((initialize == PR_TRUE) && NSS_Shutdown() != SECSuccess) {

were added to all NSS tools in bug 171263 to make sure we
catch NSS object reference leaks.  So you can certainly
adjust the exit status as appropriate.

On the other hand, I think NSS tools should call NSS_Shutdown
only if NSS was successfully initialized.
Attachment #222134 - Flags: superreview?(nelson)
Attachment #222134 - Flags: review?(wtchang)
Attachment #222134 - Flags: review?(wtchang) → review+
Comment on attachment 222134 [details] [diff] [review]
don't try to shutdown if not initialized

I wonder if any other utilities (or NSS-dependent products) will run into this.
Attachment #222134 - Flags: superreview?(nelson) → superreview+

certutil the only utility in that runs into this. Other tools might return a different error code too, but I doubt we care and hopefully no one else does. We don't document our tool return codes except for 0 meaning success.

As for other products, there is a possibility that they will. It's arguably an application bug to shutdown without initializing, though.

Checked in to the tip :

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.102; previous revision: 1.101

And to NSS_3_11_BRANCH :

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision:; previous revision:
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
Version: unspecified → 3.0
Comment on attachment 222134 [details] [diff] [review]
don't try to shutdown if not initialized

Since 'initialized' looks very close to the existing
function parameter 'initialize', I suggest that we
either rename the new variable 'inited' or use the
NSS_IsInitialized function like this:

     if ((initialize == PR_TRUE) && NSS_IsInitialized() && NSS_Shutdown() != SECSuccess) {
You need to log in before you can comment on or make changes to this bug.