1381784 - deadlock in nssSlot_IsTokenPresent, if it is called from nssSlot_GetToken

Status: RESOLVED FIXED

Description

[Daiki Ueno [:ueno]]

Attachment: devslot-lock.patch

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

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

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.

Comment 2

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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

Comment 3

[Kai Engert (:kaie:)]

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?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

(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