Closed
Bug 456406
Opened 15 years ago
Closed 15 years ago
Slot list leaks in symkeyutil
Categories
(NSS :: Tools, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
3.12.3
People
(Reporter: nelson, Assigned: julien.pierre)
Details
Attachments
(1 file, 1 obsolete file)
1.25 KB,
patch
|
nelson
:
review+
|
Details | Diff | Splinter Review |
In symkeyutil.c we see: 1034 } else { 1035 /* loop over all the slots */ 1036 PK11SlotList *slotList = PK11_GetAllTokens(CKM_INVALID_MECHANISM, 1037 PR_FALSE, PR_FALSE, &pwdata); 1038 PK11SlotListElement *se; 1039 1040 if (slotList == NULL) { 1041 PR_fprintf(PR_STDERR, "%s: No tokens found\n",progName); 1042 } 1043 for (se = PK11_GetFirstSafe(slotList); se; 1044 se=PK11_GetNextSafe(slotList,se, PR_FALSE)) { 1045 rv = ListKeys(se->slot,&printLabel,&pwdata); 1046 if (rv !=SECSuccess) { 1047 break; 1048 } 1049 } 1050 } As you can see, that code does not free the slotList, and if it breaks out of the loop, it does not free the slot list element "le". Aside: It probably should not abort the loop in the event of failure of a PKCS#11 module.
Assignee | ||
Comment 1•15 years ago
|
||
This patch frees the list. There is no need to free any individual element.
Assignee: nobody → julien.pierre.boogz
Attachment #344225 -
Flags: review?(nelson)
Reporter | ||
Comment 2•15 years ago
|
||
Comment on attachment 344225 [details] [diff] [review] free leaked list Julien, the function PK11_GetFirstSafe increments the reference count on the list element that it returns. The function PK11_GetNextSafe decrements the ref count on the list element passed in to it, and then tries to find the next element, and if it finds another list element, it increments the ref count on that element, and returns it. See this at http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#301 When PK11_GetNextSafe returns NULL, indicating end of list, then no slot list element is holding an incremented reference count. But any time that it returns a non-NULL slot list element pointer, that element has a reference that must be decremented to avoid a slot reference leak. Each list element holds a reference to a slot. when the reference count on that list element reaches zero, then the slot reference will also be released. If that ref count never reaches zero, then the slot reference is leaked. In the case of a loop that terminates before the end of the list, the code must call BOTH PK11_FreeSlotListElement and PK11_FreeSlotList. As long as a loop ALWAYS traverses the slot list all the way to the end, that is, until the point where PK11_GetNextSafe returns NULL, then all it needs to do is free the list. In that case (where it walked to the end of the list), the reference count on each list element will be 1. Then PK11_FreeSlotList will walk down the list, decrementing the reference count once on every member, and freeing the slot reference when the element ref count reaches zero. But if a function with such a loop breaks out of the loop before it reaches the end of the list, for ANY reason, then the list element it last received from PK11_GetFirstSafe or PK11_GetNextSafe will have a reference count greater than 1. Unless the code explicitly frees that list element with a call to PK11_FreeSlotListElement, before freeing the list, the slot reference held by that list element will be leaked. If the code fails to free the slot list element before freeing the list, then when it frees the list, PK11_FreeSlotList will walk down the list, decrementing the reference count once on every member, but in the case of the list member most recently returned by PK11_GetFirstSafe or PK11_GetNextSafe, that reference count will be greater than 1, and so decrementing it will not destroy the slot reference it holds. To see examples of functions that do this correctly, see PK11_GetBestSlotMultiple http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#1853 and PK11_GetMaxKeyLength http://mxr.mozilla.org/security/source/security/nss/lib/pk11wrap/pk11slot.c#1961 Note that I recently fixed a similar leak in PK11_GetMaxKeyLength. Then I went looking for functions that made this same mistake. That is how I found the code that I reported in this bug.
Attachment #344225 -
Flags: review?(nelson) → review-
Assignee | ||
Updated•15 years ago
|
Priority: -- → P2
Target Milestone: --- → 3.12.3
Assignee | ||
Comment 3•15 years ago
|
||
I was unaware of the behavior of PK11_GetNextSafe WRT element refcounts. This should be documented somewhere. It is not in header files. I believe this updated patch is now correct.
Attachment #344225 -
Attachment is obsolete: true
Attachment #345565 -
Flags: review?(nelson)
Reporter | ||
Updated•15 years ago
|
Attachment #345565 -
Flags: review?(nelson) → review+
Reporter | ||
Comment 4•15 years ago
|
||
Comment on attachment 345565 [details] [diff] [review] Free leaked list and all elements r=nelson Let me suggest that you move the declaration of 'se' down into the basic block where it is used.
Assignee | ||
Comment 5•15 years ago
|
||
Thanks for the quick review, Nelson. I checked in the fix with your suggested change. Checking in symkeyutil.c; /cvsroot/mozilla/security/nss/cmd/symkeyutil/symkeyutil.c,v <-- symkeyutil.c new revision: 1.13; previous revision: 1.12 done
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•