Closed Bug 1753315 Opened 5 months ago Closed 2 months ago

Deadlock due to reentry of module list read lock

Categories

(NSS :: Libraries, defect, P3)

All
Unspecified

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: jschanck, Assigned: jschanck)

References

(Blocks 1 open bug)

Details

(Whiteboard: [nss-fx])

Attachments

(1 file)

The module list lock serves two distinct purposes which, IMO, encourage reentry on the read lock:

 * This lock protects the global module lists.
 * it also protects changes to the slot array (module->slots[]) and slot count
 * (module->slotCount) in each module.

This is a problem because the module list lock is implemented as an NSSRW_Lock, which supports recursive write locking but not recursive read locking. Deadlock occurs if
1. thread A takes a read lock,
2. thread B queues to write, and then
3. thread A tries to take another read lock.
In this scenario, thread A will wait for the write lock to be released, even though the write lock is not held and thread A is itself preventing the write lock from being acquired.

Some Firefox shutdown hangs can be attributed to this pattern. In these cases, nsNSSComponent::HasActiveSmartCards takes a module list read lock in order to iterate over the module list (purpose 1). In each iteration it calls SECMOD_HasRemovableSlots which takes another read lock (pupose 2).

Ideally we would have separate locks for the global module list and the per-module slot lists, but this would involve changing the SECMODModule struct.

Alternatively we can make it the caller's responsibility to acquire the module list read lock for functions like SECMOD_HasRemovableSlots. I'll attach a patch which does this, but I'm open to suggestions.

Severity: -- → S2
Priority: -- → P3

So this is a public function, Other callers may already depend on SECMOD_HasRemovableSlots getting the lock itself (in fact one would expect that is what they would have done normally).

We should create a new function that doesn't hold the lock and have Firefox call the non-locking version holding the lock.

bob

Attachment #9262001 - Attachment description: Bug 1753315 - Make caller of SECMOD_HasRemovableSlots responsible for module list locking. r=#nss-reviewers → Bug 1753315 - Add SECMOD_LockedModuleHasRemovableSlots. r=#nss-reviewers
Blocks: 1763237
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.