Closed Bug 1381784 Opened 4 years ago Closed 4 years ago

deadlock in nssSlot_IsTokenPresent, if it is called from nssSlot_GetToken

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox-esr52 unaffected, firefox55 fixed)

RESOLVED FIXED
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- fixed

People

(Reporter: ueno, Assigned: ueno)

References

Details

(Keywords: regression)

Attachments

(1 file)

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: 4 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.