Closed
Bug 202593
Opened 21 years ago
Closed 21 years ago
NSS does not lock non-threadsafe modules correctly.
Categories
(NSS :: Libraries, defect, P1)
Tracking
(Not tracked)
RESOLVED
FIXED
3.8.1
People
(Reporter: rrelyea, Assigned: rrelyea)
Details
(Whiteboard: [3.7.5])
Attachments
(6 files, 1 obsolete file)
8.52 KB,
patch
|
wtc
:
review+
|
Details | Diff | Splinter Review |
2.20 KB,
patch
|
rrelyea
:
review+
|
Details | Diff | Splinter Review |
1.54 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
9.40 KB,
patch
|
Details | Diff | Splinter Review | |
9.38 KB,
patch
|
rrelyea
:
review+
bugz
:
superreview+
sspitzer
:
approval1.4+
|
Details | Diff | Splinter Review |
2.72 KB,
patch
|
bugz
:
review+
rrelyea
:
superreview+
|
Details | Diff | Splinter Review |
The PKCS #11 spec requires non-thread safe modules to be locked on a per module basis. Currenly NSS is locking on a per slot basis. There were also a case in checking slot presence where we weren't locking at all (in the Stan code). On review of some of the stan code I discovered that Stan was not handling the PKCS #11 locking semantics at all. PKCS #11 *always* requires us to lock around session, even with thread-safe modules (in the thread-safe case the lock is session dependent). In addition PKCS #11 requires us to lock all calls to the same module with the same lock if the module is not thread safe. The Pure Stan code is creating a per session lock only if the module is not thread safe.
Assignee | ||
Comment 1•21 years ago
|
||
This patch fixes the NSS 3.x code. It attempts to fix the PURE_STAN code as well, but the PURE_STAN fixes have not been tested.
Assignee | ||
Updated•21 years ago
|
Attachment #121025 -
Flags: superreview?(jpierre)
Attachment #121025 -
Flags: review?(ian.mcgreer)
Comment 2•21 years ago
|
||
Don't worry about PURE_STAN stuff. We can probably remove it. Any Stan work I've done in the last 9 months or so has been on STAN_WORK_BRANCH, so the PURE_STAN code on the tip has atrophied.
Updated•21 years ago
|
Priority: -- → P1
Target Milestone: --- → 3.8.1
Comment 3•21 years ago
|
||
This patch should be applied on top of the previous patch (attachment 121025 [details] [diff] [review]). It fixes a recursive deadlock (bug 204980) introduced by that patch. It turns out that the session returned by nssSlot_CreateSession doesn't need its own lock. It is either protected by a higher-level lock (the slot or module lock) or used by only one thread throughout its lifetime.
Updated•21 years ago
|
Attachment #122901 -
Flags: review?(relyea)
Comment 4•21 years ago
|
||
This patch should be applied on top of Bob's patch (attachment 121025 [details] [diff] [review]). We need to create slot->sessionLock before slot->freeListLock because the cleanup code before error return assumes that order. Note that this patch no longer applies to the NSS TIP after Ian removed slot->refLock.
Comment 5•21 years ago
|
||
Comment on attachment 123090 [details] [diff] [review] Incremental patch #2 Needs work I think. If mod->isThreadSave is false, and the attempt to allocate a new slot free list lock fails, then this patch destroys slot->sessionLock, which is equal to mod->refLock, thereby destroying mod->refLock. Now, I wonder if NSS will subsequently attempt to destroy mod->refLock again?
Comment 6•21 years ago
|
||
In incremental patch 2, if you change the new line that reads: + PZ_DestroyLock(slot->sessionLock); to instead read + if (mod->isThreadSafe) PZ_DestroyLock(slot->sessionLock); Then I think that incremental patch will be OK.
Assignee | ||
Comment 7•21 years ago
|
||
Comment on attachment 122901 [details] [diff] [review] Incremental patch r=relyea
Attachment #122901 -
Flags: review?(relyea) → review+
Comment 8•21 years ago
|
||
Nelson, you are right. slot->sessionLock should be destroyed only if mod->isThreadSafe is true. This patch has the change you suggested. I also deleted the unnecessary 'lock' field in the NSSToken structure.
Attachment #123090 -
Attachment is obsolete: true
Comment 9•21 years ago
|
||
Comment on attachment 121025 [details] [diff] [review] Consolidate one lock for non-thread safe modules (not one per slot). r=wtc. The problems I saw with this patch are addressed with the incremental patches I attached. Bob, please review the code again to make sure that there is no recursive deadlock when a thread-unsafe module is used because its module lock, slot lock, and session lock are the same lock.
Attachment #121025 -
Flags: review?(ian.mcgreer) → review+
Comment 10•21 years ago
|
||
Comment 11•21 years ago
|
||
Updated•21 years ago
|
Attachment #123218 -
Flags: review?(relyea)
Comment 12•21 years ago
|
||
Add self to cc list
Updated•21 years ago
|
Attachment #123158 -
Flags: review+
Comment 13•21 years ago
|
||
Bob, do we need to lock around the C_GetSlotInfo call in PK11_InitSlot, or do we assume only one thread can be in NSS when PK11_InitSlot is called? Same question for the C_CloseAllSessions call in PK11_DestroySlot.
Assignee | ||
Comment 14•21 years ago
|
||
Hmm Good point. Initialization: I know that we only initialize on one thread at startup (NSS only allows one instance of NSS_Initialize at a time). Destruction: This is another matter. It is not possible for the same Slot to be destroyed on multiple threads, *but* it is possible for multiple slots on the same Module to be destroyed on multiple threads. I don't think it's an issue for AOL Communicator, but could be an issue for Mozilla (try to switch user threads with one of these multiple slot, non-treadsafe tokens while the system is busy fetching SSL connections)... We should track that issue.
Assignee | ||
Comment 15•21 years ago
|
||
Comment on attachment 123218 [details] [diff] [review] Patch for NSS_3_8_BRANCH r=relyea good combined patch.
Attachment #123218 -
Flags: review?(relyea) → review+
Comment 16•21 years ago
|
||
Comment on attachment 123218 [details] [diff] [review] Patch for NSS_3_8_BRANCH Ian, could you review this patch today? You can ignore the changes to the PURE_STAN_CODE parts. Thanks.
Attachment #123218 -
Flags: superreview?(ian.mcgreer)
Comment 17•21 years ago
|
||
Comment on attachment 123218 [details] [diff] [review] Patch for NSS_3_8_BRANCH This seems correct. I'm a little worried about the RW session thing (should someone come along and allow a RW session outside of a function and into multiple threads), but that seems to come with the confusion of having Stan code mixed in.
Attachment #123218 -
Flags: superreview?(ian.mcgreer) → superreview+
Comment 18•21 years ago
|
||
Comment on attachment 123218 [details] [diff] [review] Patch for NSS_3_8_BRANCH Requesting mozilla 1.4 approval. This patch is needed for people who are using more than one smart cards. It does not affect people who are not using smart cards. Note that NSS patches do not require superreviews and we are using the superreview marker to mean a "second review".
Attachment #123218 -
Flags: approval1.4?
Comment 19•21 years ago
|
||
Comment on attachment 123218 [details] [diff] [review] Patch for NSS_3_8_BRANCH a=sspitzer
Attachment #123218 -
Flags: approval1.4? → approval1.4+
Comment 20•21 years ago
|
||
What Ian pointed out in comment 17 is a valid concern. We are relying on a property of the current 3.x code that R/W sessions are always created, used, and destroyed on one thread. Someone could add new code and break this property. This patch will make our code "future proof". I am not planning to check in this patch though. It is for reference only should this issue be raised again in the future. Note that nssSlot_CreateSession in 3.x has always set the session's lock to NULL. Our fix for this bug merely made that explicit and added a long comment to explain why that's safe.
Updated•21 years ago
|
Attachment #123415 -
Flags: superreview?(relyea)
Attachment #123415 -
Flags: review?(ian.mcgreer)
Updated•21 years ago
|
Attachment #123415 -
Flags: review?(ian.mcgreer) → review+
Assignee | ||
Comment 21•21 years ago
|
||
Comment on attachment 123415 [details] [diff] [review] (For reference only) create a lock for a new R/W session on a thread-safe slot Looks good r=relyea
Attachment #123415 -
Flags: superreview?(relyea) → superreview+
Comment 22•21 years ago
|
||
Fix is now in the tip (3.9), NSS_3_8_BRANCH (3.8.1), NSS_3_7_BRANCH (3.7.5), and NSS_CLIENT_TAG (mozilla 1.4 final).
Whiteboard: [3.7.5]
Comment 23•21 years ago
|
||
Marked the bug fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Updated•21 years ago
|
Attachment #121025 -
Flags: superreview?(jpierre)
You need to log in
before you can comment on or make changes to this bug.
Description
•