deadlock in nssSlot_IsTokenPresent, if it is called from nssSlot_GetToken

RESOLVED FIXED in Firefox 55

Status

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ueno, Assigned: ueno)

Tracking

({regression})

trunk
3.31
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
The recent commit:
https://hg.mozilla.org/projects/nss/rev/859a4f82207b
changed nssSlot_GetToken() to hold a lock when calling nssSlot_IsTokenPresent(), while the function also takes the same lock:

https://hg.mozilla.org/projects/nss/rev/859a4f82207b#l1.33
https://hg.mozilla.org/projects/nss/file/tip/lib/dev/devslot.c#l134

The attached patch moves the locking below to avoid possible deadlock.

Originally reported by Kostya Vasilyev in:
https://bugzilla.redhat.com/show_bug.cgi?id=1470352#c8
Attachment #8887427 - Flags: review?(ttaubert)
Attachment #8887427 - Flags: review?(dkeeler)
Comment on attachment 8887427 [details] [diff] [review]
devslot-lock.patch

Review of attachment 8887427 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM with the nit fixed. Thanks!

::: lib/dev/devslot.c
@@ +231,5 @@
> +        /* Even if a token should be present, check `slot->token` too as it
> +	 * might be gone already. This would happen mostly on shutdown. */
> +        nssSlot_EnterMonitor(slot);
> +        if (slot->token)
> +            rvToken = nssToken_AddRef(slot->token);

Please use braces even for a single-line in the body of the if-statement.
Attachment #8887427 - Flags: review?(ttaubert) → review+
Comment on attachment 8887427 [details] [diff] [review]
devslot-lock.patch

Review of attachment 8887427 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM (clearing r? because I'm not an NSS peer - Tim's r+ should be sufficient, I believe.)
Attachment #8887427 - Flags: review?(dkeeler)
Thanks Daiki!
https://hg.mozilla.org/projects/nss/rev/58026f3ade78

Do we expect this to affect many users?

Does this justify a NSS 3.31.x release, or can this wait for 3.32 which will probably get release around August 3rd?
Assignee: nobody → dueno
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.32
(In reply to Kai Engert (:kaie:) (on vacation) from comment #3)
> Does this justify a NSS 3.31.x release, or can this wait for 3.32 which will
> probably get release around August 3rd?

Looks like it does...

https://hg.mozilla.org/projects/nss/rev/8dd4fb083510
Target Milestone: 3.32 → 3.31
Duplicate of this bug: 1372621
You need to log in before you can comment on or make changes to this bug.