Closed Bug 335036 Opened 15 years ago Closed 15 years ago

NSS_Shutdown() does not check that NSS is initialized

Categories

(NSS :: Libraries, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
3.11.2

People

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

Details

Attachments

(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 all.sh . One of the tests in dbtests.sh 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
Status: NEW → ASSIGNED
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

r=nelson
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
done

And to NSS_3_11_BRANCH :

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

The new error code was added as part of the checkin to the fix for bug 337789 .
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.11.2
One of the tests in dbtests.sh, "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) {
        exit(1);
    }

    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 ?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Statements like:

    if ((initialize == PR_TRUE) && NSS_Shutdown() != SECSuccess) {
        exit(1);
    }

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+
Nelson,

certutil the only utility in all.sh 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
done

And to NSS_3_11_BRANCH :

Checking in certutil.c;
/cvsroot/mozilla/security/nss/cmd/certutil/certutil.c,v  <--  certutil.c
new revision: 1.97.2.4; previous revision: 1.97.2.3
done
Status: REOPENED → RESOLVED
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) {
         exit(1);
     }
You need to log in before you can comment on or make changes to this bug.