Last Comment Bug 202593 - NSS does not lock non-threadsafe modules correctly.
: NSS does not lock non-threadsafe modules correctly.
Status: RESOLVED FIXED
[3.7.5]
:
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.8
: x86 Windows 2000
: P1 normal (vote)
: 3.8.1
Assigned To: Robert Relyea
: Bishakha Banerjee
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2003-04-18 16:30 PDT by Robert Relyea
Modified: 2003-10-02 17:52 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Consolidate one lock for non-thread safe modules (not one per slot). (8.52 KB, patch)
2003-04-18 16:32 PDT, Robert Relyea
wtc: review+
Details | Diff | Review
Incremental patch (2.20 KB, patch)
2003-05-09 17:30 PDT, Wan-Teh Chang
rrelyea: review+
Details | Diff | Review
Incremental patch #2 (1.02 KB, patch)
2003-05-12 18:43 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Incremental patch #2 v1.1 (1.54 KB, patch)
2003-05-13 11:36 PDT, Wan-Teh Chang
nelson: review+
Details | Diff | Review
Patch for NSS_3_7_BRANCH (9.40 KB, patch)
2003-05-13 17:10 PDT, Wan-Teh Chang
no flags Details | Diff | Review
Patch for NSS_3_8_BRANCH (9.38 KB, patch)
2003-05-13 17:50 PDT, Wan-Teh Chang
rrelyea: review+
bugz: superreview+
sspitzer: approval1.4+
Details | Diff | Review
(For reference only) create a lock for a new R/W session on a thread-safe slot (2.72 KB, patch)
2003-05-15 10:23 PDT, Wan-Teh Chang
bugz: review+
rrelyea: superreview+
Details | Diff | Review

Description Robert Relyea 2003-04-18 16:30:23 PDT
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.
Comment 1 Robert Relyea 2003-04-18 16:32:26 PDT
Created attachment 121025 [details] [diff] [review]
Consolidate one lock for non-thread safe modules (not one per slot).

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.
Comment 2 Ian McGreer 2003-04-21 14:23:48 PDT
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.
Comment 3 Wan-Teh Chang 2003-05-09 17:30:25 PDT
Created attachment 122901 [details] [diff] [review]
Incremental patch

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.
Comment 4 Wan-Teh Chang 2003-05-12 18:43:48 PDT
Created attachment 123090 [details] [diff] [review]
Incremental patch #2

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 Nelson Bolyard (seldom reads bugmail) 2003-05-12 20:25:59 PDT
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 Nelson Bolyard (seldom reads bugmail) 2003-05-12 22:17:47 PDT
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.
Comment 7 Robert Relyea 2003-05-13 07:53:53 PDT
Comment on attachment 122901 [details] [diff] [review]
Incremental patch

r=relyea
Comment 8 Wan-Teh Chang 2003-05-13 11:36:59 PDT
Created attachment 123158 [details] [diff] [review]
Incremental patch #2 v1.1

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.
Comment 9 Wan-Teh Chang 2003-05-13 13:01:40 PDT
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.
Comment 10 Wan-Teh Chang 2003-05-13 17:10:57 PDT
Created attachment 123214 [details] [diff] [review]
Patch for NSS_3_7_BRANCH
Comment 11 Wan-Teh Chang 2003-05-13 17:50:49 PDT
Created attachment 123218 [details] [diff] [review]
Patch for NSS_3_8_BRANCH
Comment 12 Nelson Bolyard (seldom reads bugmail) 2003-05-13 21:58:39 PDT
Add self to cc list
Comment 13 Wan-Teh Chang 2003-05-14 07:47:47 PDT
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.
Comment 14 Robert Relyea 2003-05-14 08:57:36 PDT
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.
Comment 15 Robert Relyea 2003-05-14 09:02:57 PDT
Comment on attachment 123218 [details] [diff] [review]
Patch for NSS_3_8_BRANCH

r=relyea good combined patch.
Comment 16 Wan-Teh Chang 2003-05-14 10:45:55 PDT
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.
Comment 17 Ian McGreer 2003-05-14 13:53:29 PDT
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.
Comment 18 Wan-Teh Chang 2003-05-14 15:44:52 PDT
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".
Comment 19 (not reading, please use seth@sspitzer.org instead) 2003-05-15 09:38:45 PDT
Comment on attachment 123218 [details] [diff] [review]
Patch for NSS_3_8_BRANCH

a=sspitzer
Comment 20 Wan-Teh Chang 2003-05-15 10:23:53 PDT
Created attachment 123415 [details] [diff] [review]
(For reference only) create a lock for a new R/W session on a thread-safe slot

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.
Comment 21 Robert Relyea 2003-05-15 15:07:56 PDT
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
Comment 22 Wan-Teh Chang 2003-05-15 19:20:37 PDT
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).
Comment 23 Wan-Teh Chang 2003-10-02 17:52:58 PDT
Marked the bug fixed.

Note You need to log in before you can comment on or make changes to this bug.