Last Comment Bug 341122 - Coverity 633 SFTK_DestroySlotData uses slot->slotLock then checks it for NULL
: Coverity 633 SFTK_DestroySlotData uses slot->slotLock then checks it for NULL
Status: RESOLVED FIXED
: coverity
Product: NSS
Classification: Components
Component: Libraries (show other bugs)
: 3.11.1
: All All
: P3 minor (vote)
: 3.12
Assigned To: Ryan Jones
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-06-10 23:02 PDT by Nelson Bolyard (seldom reads bugmail)
Modified: 2006-10-23 15:59 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch v1 (1.26 KB, patch)
2006-10-18 09:42 PDT, Ryan Jones
nelson: review-
Details | Diff | Review
Patch v2 (957 bytes, patch)
2006-10-18 13:11 PDT, Ryan Jones
nelson: review+
wtc: superreview+
Details | Diff | Review

Description Nelson Bolyard (seldom reads bugmail) 2006-06-10 23:02:43 PDT
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 2006-10-18 09:42:59 PDT
Created attachment 242653 [details] [diff] [review]
Patch v1

Patch v1.

Move |SFTK_ShutdownSlot(slot)| after the |if (slot->slotLock)| block.
Comment 2 Nelson Bolyard (seldom reads bugmail) 2006-10-18 13:03:19 PDT
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 2006-10-18 13:11:06 PDT
Created attachment 242681 [details] [diff] [review]
Patch v2

Patched too Nelson Bolyard's comment; removed the if check and preserve code structure.
Comment 4 Nelson Bolyard (seldom reads bugmail) 2006-10-18 16:19:30 PDT
Comment on attachment 242681 [details] [diff] [review]
Patch v2

r=nelson for trunk
Comment 5 Wan-Teh Chang 2006-10-23 15:58:58 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.