273637 - 3 locks in softoken have unsafe initialization

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Julien Pierre]

As reported by Nelson in bugzilla 270689, comment 7, there are some more locks
in NSS which have unsafe late allocation. They revolve mostly around the use of
nss_InitLock .

Comment 1

[Julien Pierre]

The other potentially unsafe lock allocations are in 
1) in keydb.c, keydb_InitLocks, which is called from nsslowkey_NewHandle
2) in pcertdb.c, certdb_InitDBLock, which is called from nsslowcert_OpenCertDB

I verified that thes above 2 are created during NSC_Initialize execution, so they are OK .

3) in pcertdb.c, in nsslowcert_LockFreeList
4) in pcertdb.c, in nsslowcert_LockCertRefCount
5) in pcertdb.c, in nsslowcert_LockCertTrust

However, these last 3 lock creations are not safe. The locks don't get created in C_Initialize . In a simple program that just did NSS_Init / NSS_Shutdown, the FreeList lock got created in NSC_Finalize . The other 2 locks were never created ! There is potential for race conditions if the application spawns threads before the locks are created. Thus, I'm making this P1 for 3.11 . The fix is to initialize these 3 locks early on, from C_Initialize .

Comment 2

[Julien Pierre]

Bob, is there any chance this could be fixed by the end of the week ?

Comment 3

[Julien Pierre]

Per our meeting today, latering to 3.11.1 .

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Alexei, 

Could this be the cause of any of the bugs we're now seeing in QA stress testing? 
Would you like to take this bug?  
It basically involves moving the code that initializes and destroys certain
locks in softoken, so that the locks are created in NSC_Initialize and 
destroyed in NSC_Finalize (I think).

Comment 5

[Robert Relyea]

Attachment: Create an init function to initialize global locks rather than initing them on the fly

Comment 6

[Robert Relyea]

Ignore the EC_ParamFill change in pkcs11.c,
I'm getting to many outstanding softoken patches.... all of them small...

Comment 7

[Nelson Bolyard (seldom reads bugmail)]

Bob, Please generate patches where the reviewers don't have to figure out
what parts of the patches to ignore.  This problem occurs too frequently.
Please Pull a separate tree for each patch. It's not difficult or 
expensive or particularly time consuming, IMO.

Also, diff -u is really necessary.  

Comment 8

[Robert Relyea]

Attachment: Create an init function to initialize global locks rather than initing them on the fly

-u version of the bug

Comment 9

[Robert Relyea]

Bob, Please generate patches where the reviewers don't have to figure out
what parts of the patches to ignore.  This problem occurs too frequently.
Please Pull a separate tree for each patch. It's not difficult or 
expensive or particularly time consuming, IMO.

I do not intentionally do this, but it's inevitiable if there are more than one patch in the same directory. The longer I have to hold a patch in my tree, the more likely this kind of collision happens.

Once there is a collision managing that is extremely error prone. I think once we get through 3.11 we need to review our review system.

bob

Comment 10

[Andreas Sterbenz]

(In reply to comment #8)
> Created an attachment (id=215415) [edit]
> Create an init function to initialize global locks rather than initing them on
> the fly

I tried to mark this reviewed+, but bugzilla tells me "You are not authorised to edit attachment 215415 [details] [diff] [review]."

BTW, I am not particularly familiar with this code, not sure why I was asked to review.

Comment 11

[Robert Relyea]

Comment on attachment 215415 [details] [diff] [review]
Create an init function to initialize global locks rather than initing them on the fly

Sorry Andreas. I meant to ask alexi to review it. I'm trying to spread out my review requests so they all don't fall on a single person.

alexi, this one should be isolated enough fore a review.

Comment 12

[Alexei Volkov]

Comment on attachment 215415 [details] [diff] [review]
Create an init function to initialize global locks rather than initing them on the fly

Looks good. One thing with PR_Assert: it does not do anything for optimized builds as far as I know. Depends on your policy for library code, you may want to essue some warnings if thread got to lock function with uninitialized lock(s).

Comment 13

[Julien Pierre]

Comment on attachment 215415 [details] [diff] [review]
Create an init function to initialize global locks rather than initing them on the fly

I think this patch works from a functional point of view. I find it odd however  that we have nsslowcert_InitLocks initialize three locks  - freeListLock, certRefCountLock, and certTrustLock - and then nsslowcert_DestroyLocks (which this patch did not change) destroys dbLock, certRefCountLock and certTrustLock; while freeListLock is destroyed as part of nsslowcert_DestroyFreeLists .

It would make a lot more sense if freeListLock was allocated and freed in the same functions as certRefCountLock and certTrustLock . I'm not too sure why dbLock is allocated in a separate function than all the others - certdb_InitDBLock . This all could use some merging, though.

Comment 14

[Robert Relyea]

Checked into tip and 3.11
---------------------------- tip ------------------------------------
Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.116; previous revision: 1.115
done
Checking in keydb.c;
/cvsroot/mozilla/security/nss/lib/softoken/keydb.c,v  <--  keydb.c
new revision: 1.41; previous revision: 1.40
done
Checking in pcertdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/pcertdb.c,v  <--  pcertdb.c
new revision: 1.58; previous revision: 1.57
done
------------- 3.11 --------------------
Checking in pcert.h;
/cvsroot/mozilla/security/nss/lib/softoken/pcert.h,v  <--  pcert.h
new revision: 1.11.2.1; previous revision: 1.11
done
Checking in pcertdb.c;
/cvsroot/mozilla/security/nss/lib/softoken/pcertdb.c,v  <--  pcertdb.c
new revision: 1.53.2.4; previous revision: 1.53.2.3
done
Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.112.2.3; previous revision: 1.112.2.2
done