278276 - Slot List Elements cannot be freed by applications.

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Robert Relyea]

There are a set of helpler functions for safely traversing SlotLists. These
functions return SlotListElements, which have their own references. When
iterating through a full tree of slot lists, the references are handled
correctly, because the GetNext() or GetNextSafe() functions automatically
decrement the slot list.

Inside of NSS itself, the references are handled correctly because they can call
the pk11_FreeSlotListElement() private function.

This function, however, is not exported, so applications which write code like:

for (le = PK11_GetFirstSafe(slotList); le; le =
PK11_GetNextSafe(slotList,le,PR_FALSE)) {
    if (le->slot == MySlot) {
        break;
    }
}

will leak a reference to le.

The solution is to export a public function equivalent to pk11_FreeSlotListElement.

There is at least one PSM leak of this time.

Comment 1

[Robert Relyea]

Attachment: Export the function to free a slot list element.

This patch not only exports the function, but also removes some redundant code
I found when I was writting this patch.

Comment 2

[Julien Pierre]

Comment on attachment 188486 [details] [diff] [review]
Export the function to free a slot list element.

Everything looks mostly good, except for one thing. 

The new public function should not return void. It should return a SECStatus.
This should be used to return an error in case the argument passed in (eg. list
pointer) is NULL or if some other error occurs while walking the list. This
way, the freeing of the list will not silently fail in applications.

Comment 3

[Wan-Teh Chang]

Comment on attachment 188486 [details] [diff] [review]
Export the function to free a slot list element.

Bob, did you compile the code?	The PK11_FreeSlotListElement
declaration in pk11pub.h only takes one argument.

Other than that, this patch seems correct.

It would be good to explain what the Static in
pk11_InitSlotListStatic and pk11_FreeSlotListStatic
mean.  At first I thought it means the functions are
static functions.  Only after reading the whole patch
did I know that it means the PK11SlotList *list function
argument points to a static variable (as opposed one
allocated from the heap).

Comment 4

[Julien Pierre]

Comment on attachment 188486 [details] [diff] [review]
Export the function to free a slot list element.

Resetting the review flags to what they should be :).

Comment 5

[Robert Relyea]

Attachment: Incorporate Julien and wtc's comments

Comment 6

[Julien Pierre]

Comment on attachment 188577 [details] [diff] [review]
Incorporate Julien and wtc's comments

IMO, the callers of PK11_FreeSlotListElement should check its return status and
take action upon failure.

That includes PK11_DeleteSlotFromList , pk11_FreeSlotListStatic (which should
return SECStatus instead of void), PK11_GetNextRef, PK11_GetNextSafe,
PK11_ClearSlotList, PK11_GetBestSlotMultiple.
Unfortunately, PK11_FreeSlotList is already declared as returning void :(

I'm OK with this patch as long as you open another bug to track this problem.

Comment 7

[Wan-Teh Chang]

Comment on attachment 188577 [details] [diff] [review]
Incorporate Julien and wtc's comments

r=wtc.

Having PK11_FreeSlotListElement return SECStatus is
a gratuitous change.  Nobody will bother to check the
return value.

NSS calls PK11_FreeSlotListElement in six places
and never checks its return value.  This is because
you can verify that the "list" and "le" arguments
cannot be NULL in any of those places.	This is likely
to be true for new code that uses PK11_FreeSlotListElement.

Comment 8

[Robert Relyea]

wtchang: review+
cvs commit pk11slot.c
Checking in pk11slot.c;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11slot.c,v  <--  pk11slot.c
new revision: 1.85; previous revision: 1.84
Checking in pk11pub.h;
/cvsroot/mozilla/security/nss/lib/pk11wrap/pk11pub.h,v  <--  pk11pub.h
new revision: 1.8; previous revision: 1.7
done
Checking in nss.def;
/cvsroot/mozilla/security/nss/lib/nss/nss.def,v  <--  nss.def
new revision: 1.152; previous revision: 1.151
done
relyea(304)