NSS_Shutdown() does not check that NSS is initialized

RESOLVED FIXED in 3.11.2

Status

NSS
Libraries
P2
normal
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Julien Pierre, Assigned: Julien Pierre)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

2.96 KB, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Alexei Volkov
: superreview+
Details | Diff | Splinter Review
2.24 KB, patch
Wan-Teh Chang
: review+
Nelson Bolyard (seldom reads bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
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)

Comment 1

12 years ago
Created attachment 219396 [details] [diff] [review]
fail with SEC_ERROR_NOT_INITIALIZED if there is no NSS to shut down
Assignee: nobody → julien.pierre.bugs
Status: NEW → ASSIGNED
Attachment #219396 - Flags: review?(nelson)
(Assignee)

Updated

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

Comment 3

12 years ago
Created attachment 222075 [details] [diff] [review]
incorporate Nelson's feedback

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+

Updated

12 years ago
Attachment #222075 - Flags: superreview?(alexei.volkov.bugs) → superreview+
(Assignee)

Comment 5

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

Updated

12 years ago
Target Milestone: --- → 3.11.2
(Assignee)

Comment 6

12 years ago
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 → ---

Comment 7

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

Comment 8

12 years ago
Created attachment 222134 [details] [diff] [review]
don't try to shutdown if not initialized
Attachment #222134 - Flags: superreview?(nelson)
Attachment #222134 - Flags: review?(wtchang)

Updated

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

Comment 10

12 years ago
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
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
Version: unspecified → 3.0

Comment 11

12 years ago
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.