202593 - NSS does not lock non-threadsafe modules correctly.

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P1)

Description

[Robert Relyea]

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]

Attachment: 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]

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]

Attachment: 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]

Attachment: 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)]

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)]

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]

Comment on attachment 122901 [details] [diff] [review]
Incremental patch

r=relyea

Comment 8

[Wan-Teh Chang]

Attachment: 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]

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]

Attachment: Patch for NSS_3_7_BRANCH

Comment 11

[Wan-Teh Chang]

Attachment: Patch for NSS_3_8_BRANCH

Comment 12

[Nelson Bolyard (seldom reads bugmail)]

Add self to cc list

Comment 13

[Wan-Teh Chang]

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]

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]

Comment on attachment 123218 [details] [diff] [review]
Patch for NSS_3_8_BRANCH

r=relyea good combined patch.

Comment 16

[Wan-Teh Chang]

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]

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]

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 [e-mail] instead)]

Comment on attachment 123218 [details] [diff] [review]
Patch for NSS_3_8_BRANCH

a=sspitzer

Comment 20

[Wan-Teh Chang]

Attachment: (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]

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]

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]

Marked the bug fixed.