GetSlotWithMechanism leaks slot list element in several paths

RESOLVED FIXED

Status

()

Core
Security: PSM
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Nelson Bolyard (seldom reads bugmail), Assigned: timeless)

Tracking

Trunk
x86
Windows XP
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.29 KB, patch
kaie
: review+
Details | Diff | Splinter Review
In manager/ssl/src/nsKeygenHandler.cpp function GetSlotWithMechanism 
near line 439, we see a loop that calls PK11_GetNextSafe until it returns
NULL, or until a NULL is found in an array, which (according to the comments)
signifies an OOM error.  That OOM error code path jumps out of the loop,
leaking the slotElement (which is normally freed by the next call to 
PK11_GetNextSafe).  It should call PK11_FreeSlotListElement to free the
slotElement before jumping out of the loop.

I doubt this is happening frequently, but if it did happen, the leak would
prevent the PKCS#11 module for the list element from being C_Finalized at 
shutdown time.  Bug 452751 is an example of the kind of problem that occurs
when that happens.
Also in that same function, near line 482, we see another loop that calls
PK11_GetNextSafe until either 
(a) that function returns NULL, or 
(b) a match is found on the token name.  

This probably breaks out of the loop commonly (unlike the code in comment 0).
When it does so, it leaks the slotElement.  This conceivably might be the 
cause of bug 452751.
Summary: GetSlotWithMechanism leaks slot list element in OOM error path → GetSlotWithMechanism leaks slot list element in several paths
(Assignee)

Comment 2

9 years ago
Created attachment 354570 [details] [diff] [review]
various bits
Assignee: kaie → timeless
Status: NEW → ASSIGNED
Attachment #354570 - Flags: review?(nelson)
Comment on attachment 354570 [details] [diff] [review]
various bits

Here are some comments on this patch.  I will not give it an r+ or r-, 
but will refer it to the PSM module owner for that purpose.

>@@ -476,6 +477,7 @@ GetSlotWithMechanism(PRUint32 aMechanism
>         nsAutoString tokenStr(unicodeTokenChosen);
>         while (slotElement) {
>             if (tokenStr.Equals(NS_ConvertUTF8toUTF16(PK11_GetTokenName(slotElement->slot)))) {
>+                PK11_FreeSlotListElement(slotList, slotElement);
>                 *aSlot = slotElement->slot;
>                 break;
>             }

The above patch frees the slotElement, and then uses the slotElement it 
just freed.  Change the order of those two lines, so that you take the 
slot address out of the element before freeing it.


>@@ -530,8 +530,15 @@ NS_IMETHODIMP nsPK11TokenDB::ListTokens(
> 
>   for (le = PK11_GetFirstSafe(list); le; le = PK11_GetNextSafe(list, le, PR_FALSE)) {
>     nsCOMPtr<nsIPK11Token> token = new nsPK11Token(le->slot);
>+    rv = token
>+      ? array->AppendElement(token)
>+      : NS_ERROR_OUT_OF_MEMORY;
> 
>-    array->AppendElement(token);
>+    if (NS_FAILED(rv)) {
>+      PK11_FreeSlotListElement(list, le);
>+      rv = NS_ERROR_OUT_OF_MEMORY;
>+      goto done;
>+    }
>   }

Is the only possible cause of token being NULL an OOM?
What about a slot that has no token?
In general, functions whose job is to traverse the slots looking
for a token with certain properties should NOT abort the search
because they ecounter a slot with a problematic token.  They 
should just ignore it and go on.  Now, IFF this really is an OOM
condition, and we're sure that there's no point in going on, 
because any further attempts to get tokens for subsequent slots 
will also fail, then this is OK.  Otherwise, it isn't.

>--- a/security/manager/ssl/src/nsSmartCardMonitor.cpp
>+++ b/security/manager/ssl/src/nsSmartCardMonitor.cpp

This patch to nsSmartCardMonitor.cpp seems unrelated to this bug.
I did not review it.
Attachment #354570 - Flags: review?(nelson) → review?(kaie)
(Assignee)

Comment 4

9 years ago
Created attachment 355258 [details] [diff] [review]
per nelson
Attachment #354570 - Attachment is obsolete: true
Attachment #355258 - Flags: review?(kaie)
Attachment #354570 - Flags: review?(kaie)
(Assignee)

Comment 5

9 years ago
yes the only way it can fail is the one line which you quoted in context:

>     nsCOMPtr<nsIPK11Token> token = new nsPK11Token(le->slot);
Version: 1.9.0 Branch → Trunk

Comment 6

8 years ago
Comment on attachment 355258 [details] [diff] [review]
per nelson

r=kaie, but please make a simple change:

                 // OOM. adjust numSlots so we don't free unallocated memory. 
+                PK11_FreeSlotListElement(slotList, slotElement);
                 numSlots = i;

Please move your new code one line down.
The existing comment and existing assignment should remain next to each other.
Attachment #355258 - Flags: review?(kaie) → review+

Updated

8 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [c-n: needs updated patch per comment 6]
(Assignee)

Comment 7

8 years ago
http://hg.mozilla.org/mozilla-central/rev/43ee697bd867
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [c-n: needs updated patch per comment 6]
You need to log in before you can comment on or make changes to this bug.