Closed Bug 202593 Opened 21 years ago Closed 21 years ago

NSS does not lock non-threadsafe modules correctly.

Categories

(NSS :: Libraries, defect, P1)

x86
Windows 2000
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rrelyea, Assigned: rrelyea)

Details

(Whiteboard: [3.7.5])

Attachments

(6 files, 1 obsolete file)

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.
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.
Attachment #121025 - Flags: superreview?(jpierre)
Attachment #121025 - Flags: review?(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.
Priority: -- → P1
Target Milestone: --- → 3.8.1
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.
Attachment #122901 - Flags: review?(relyea)
Attached patch Incremental patch #2 (obsolete) — Splinter Review
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 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?
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 on attachment 122901 [details] [diff] [review]
Incremental patch

r=relyea
Attachment #122901 - Flags: review?(relyea) → review+
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 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+
Attachment #123218 - Flags: review?(relyea)
Add self to cc list
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.
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 on attachment 123218 [details] [diff] [review]
Patch for NSS_3_8_BRANCH

r=relyea good combined patch.
Attachment #123218 - Flags: review?(relyea) → review+
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 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 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 on attachment 123218 [details] [diff] [review]
Patch for NSS_3_8_BRANCH

a=sspitzer
Attachment #123218 - Flags: approval1.4? → approval1.4+
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.
Attachment #123415 - Flags: superreview?(relyea)
Attachment #123415 - Flags: review?(ian.mcgreer)
Attachment #123415 - Flags: review?(ian.mcgreer) → review+
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+
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]
Marked the bug fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Attachment #121025 - Flags: superreview?(jpierre)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: