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

RESOLVED FIXED in 3.12

Status

NSS
Libraries
P3
minor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: sciguyryan)

Tracking

({coverity})

3.11.1
3.12
coverity

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

957 bytes, patch
Nelson Bolyard (seldom reads bugmail)
: review+
Wan-Teh Chang
: superreview+
Details | Diff | Splinter Review
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.
(Reporter)

Updated

11 years ago
Priority: -- → P3
Target Milestone: --- → 3.11.3
(Assignee)

Updated

11 years ago
(Assignee)

Comment 1

11 years ago
Created attachment 242653 [details] [diff] [review]
Patch v1

Patch v1.

Move |SFTK_ShutdownSlot(slot)| after the |if (slot->slotLock)| block.
Assignee: nobody → sciguyryan+bugzilla
Status: NEW → ASSIGNED
Attachment #242653 - Flags: superreview?(wtchang)
Attachment #242653 - Flags: review?(wtchang)
(Reporter)

Comment 2

11 years ago
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.
Attachment #242653 - Flags: review?(wtchang) → review-
(Assignee)

Comment 3

11 years ago
Created attachment 242681 [details] [diff] [review]
Patch v2

Patched too Nelson Bolyard's comment; removed the if check and preserve code structure.
Attachment #242653 - Attachment is obsolete: true
Attachment #242681 - Flags: superreview?(wtchang)
Attachment #242681 - Flags: review?(nelson)
Attachment #242653 - Flags: superreview?(wtchang)
(Reporter)

Comment 4

11 years ago
Comment on attachment 242681 [details] [diff] [review]
Patch v2

r=nelson for trunk
Attachment #242681 - Flags: review?(nelson) → review+

Comment 5

11 years ago
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.
Attachment #242681 - Flags: superreview?(wtchang) → superreview+

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Target Milestone: 3.11.3 → 3.12
You need to log in before you can comment on or make changes to this bug.