335036 - NSS_Shutdown() does not check that NSS is initialized

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P2)

Description

[Julien Pierre]

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 .

Comment 1

[Julien Pierre]

Attachment: fail with SEC_ERROR_NOT_INITIALIZED if there is no NSS to shut down

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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-

Comment 3

[Julien Pierre]

Attachment: incorporate Nelson's feedback

Also define a string for SEC_ERROR_CRL_ALREADY_EXISTS, which didn't have one yet.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 222075 [details] [diff] [review]
incorporate Nelson's feedback

r=nelson

Comment 5

[Julien Pierre]

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 .

Comment 6

[Julien Pierre]

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 ?

Comment 7

[Wan-Teh Chang]

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.

Comment 8

[Julien Pierre]

Attachment: don't try to shutdown if not initialized

Comment 9

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 10

[Julien Pierre]

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

Comment 11

[Wan-Teh Chang]

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);
     }