456406 - Slot list leaks in symkeyutil

Status: RESOLVED FIXED

(NSS :: Tools, defect, P2)

Description

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 1

[Julien Pierre]

Attachment: free leaked list

This patch frees the list. There is no need to free any individual element.

Comment 2

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 3

[Julien Pierre]

Attachment: Free leaked list and all elements

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.

Comment 4

[Nelson Bolyard (seldom reads bugmail)]

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.

Comment 5

[Julien Pierre]

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