Closed
Bug 456409
Opened 16 years ago
Closed 14 years ago
GetSlotWithMechanism leaks slot list element in several paths
Categories
(Core :: Security: PSM, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: nelson, Assigned: timeless)
Details
Attachments
(1 file, 1 obsolete file)
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.
Reporter | ||
Comment 1•16 years ago
|
||
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
Reporter | ||
Comment 3•16 years ago
|
||
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)
Attachment #354570 -
Attachment is obsolete: true
Attachment #355258 -
Flags: review?(kaie)
Attachment #354570 -
Flags: review?(kaie)
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•15 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•15 years ago
|
Keywords: checkin-needed
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [c-n: needs updated patch per comment 6]
http://hg.mozilla.org/mozilla-central/rev/43ee697bd867
Status: ASSIGNED → RESOLVED
Closed: 14 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.
Description
•