341122 - Coverity 633 SFTK_DestroySlotData uses slot->slotLock then checks it for NULL

Status: RESOLVED FIXED

(NSS :: Libraries, defect, P3)

Description

[Nelson Bolyard (seldom reads bugmail)]

Coverity 633 
SFTK_DestroySlotData first calls 

2803 	    SFTK_ShutdownSlot(slot);

which locks slot->slotLock, then later SFTK_DestroySlotData checks 
slot->slotLock for a NULL Value

2824 	    if (slot->slotLock) {
2825 		PZ_DestroyLock(slot->slotLock);
2826 		slot->slotLock = NULL;
2827 	    }

Coverity thinks that either the slot->slotLock test should be done before 
calling SFTK_ShutdownSlot or else PZ_DestroyLock should be called 
unconditionally.

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v1

Patch v1.

Move |SFTK_ShutdownSlot(slot)| after the |if (slot->slotLock)| block.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242653 [details] [diff] [review]
Patch v1

I think the order of operations in the existing code is correct.  PZ_DestroyLock should be called unconditionally.

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

Attachment: Patch v2

Patched too Nelson Bolyard's comment; removed the if check and preserve code structure.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

Comment on attachment 242681 [details] [diff] [review]
Patch v2

r=nelson for trunk

Comment 5

[Wan-Teh Chang]

Comment on attachment 242681 [details] [diff] [review]
Patch v2

r=wtc.  I checked in the patch on the NSS trunk (NSS 3.12).

Checking in pkcs11.c;
/cvsroot/mozilla/security/nss/lib/softoken/pkcs11.c,v  <--  pkcs11.c
new revision: 1.136; previous revision: 1.135
done

This is one of those patches that are correct but I am not
sure if we should check in.

If you look at the code under the comment "now we can finally
get rid of the locks", you'll see that they all have the same
pattern:

    if (<lock>) {
        PZ_DestroyLock(<lock>);
        <lock>;
    }

This patch breaks that symmetry in the code.  It seems that
we should change all of them or don't change any of them.