Closed Bug 341122 Opened 18 years ago Closed 18 years ago

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

Categories

(NSS :: Libraries, defect, P3)

3.11.1

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nelson, Assigned: sciguyryan)

References

()

Details

(Keywords: coverity)

Attachments

(1 file, 1 obsolete file)

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.
Priority: -- → P3
Target Milestone: --- → 3.11.3
Attached patch Patch v1 (obsolete) — Splinter Review
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)
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-
Attached patch Patch v2Splinter Review
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)
Comment on attachment 242681 [details] [diff] [review]
Patch v2

r=nelson for trunk
Attachment #242681 - Flags: review?(nelson) → review+
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+
Status: ASSIGNED → RESOLVED
Closed: 18 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.

Attachment

General

Created:
Updated:
Size: