Closed
Bug 283563
Opened 19 years ago
Closed 19 years ago
OOM crash [@ GetSlotWithMechanism][@ nsKeygenFormProcessor::GetPublicKey]
Categories
(Core Graveyard :: Security: UI, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: timeless, Assigned: bastiaan)
References
()
Details
(Keywords: crash, memory-leak)
Crash Data
Attachments
(1 file, 4 obsolete files)
6.04 KB,
patch
|
benjamin
:
approval1.8b4+
|
Details | Diff | Splinter Review |
in addition to assigning to unchecked allocations, i believe there are cases where FFMs will occur. i'm also fairly certain there are memory leaks floating around, possibly in standard codepaths in addition to the error paths (which seem to be such that you can't write proper error handling).
Assignee | ||
Comment 1•19 years ago
|
||
Assignee: kaie → b.jacques
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•19 years ago
|
||
Attachment #188485 -
Attachment is obsolete: true
Assignee | ||
Updated•19 years ago
|
Attachment #188493 -
Flags: review?(kaie)
Assignee | ||
Comment 3•19 years ago
|
||
Attachment #188493 -
Attachment is obsolete: true
Attachment #188497 -
Flags: review?(kaie)
Assignee | ||
Updated•19 years ago
|
Attachment #188493 -
Flags: review?(kaie)
Comment 4•19 years ago
|
||
Comment on attachment 188497 [details] [diff] [review] EVEN better fix ;) Thanks for your patch, good work, but I'd like to request some changes. --- You are introducing reallocating attempts on OOM, but you don't check whether the reallocation succeeds (tokenNameList). In my opinion, if we are OOM, we should just fail. (Chances are not high the reallocation will succeed, and the system is already in trouble anyway, so let's not do a lot of reallocation, but let's stop our action.) In my opinion, we should not try to operate on a data subset in OOM. --- Please make sure pkac.challenge.data is initialized to zero very early, so we won't try to free a random pointer.
Attachment #188497 -
Flags: review?(kaie) → review-
Assignee | ||
Comment 5•19 years ago
|
||
Attachment #188497 -
Attachment is obsolete: true
Attachment #188666 -
Flags: review?(kaie)
Comment 6•19 years ago
|
||
Comment on attachment 188666 [details] [diff] [review] address comments r=kaie
Attachment #188666 -
Flags: review?(kaie) → review+
Assignee | ||
Updated•19 years ago
|
Attachment #188666 -
Flags: superreview?(darin)
Comment 7•19 years ago
|
||
Comment on attachment 188666 [details] [diff] [review] address comments >Index: security/manager/ssl/src/nsKeygenHandler.cpp >+ tokenNameList[i] = ToNewUnicode(NS_ConvertUTF8toUCS2(PK11_GetTokenName(slotElement->slot))); nit: change this to use UTF8ToNewUnicode instead to eliminate one string copy. > if (NS_FAILED(rv)) goto loser; > > { > nsPSMUITracker tracker; >- if (tracker.isUIForbidden()) { >+ if (!tokenNameList || !*tokenNameList) { >+ rv = NS_ERROR_OUT_OF_MEMORY; >+ } >+ else if (tracker.isUIForbidden()) { > rv = NS_ERROR_NOT_AVAILABLE; > } > else { > rv = dialogs->ChooseToken(nsnull, (const PRUnichar**)tokenNameList, numSlots, &unicodeTokenChosen, &canceled); > } nit: indentation is being changed by this patch. i think you should stick to the indentation style of the existing file. Otherwise, this patch looks fine to me. Please fix those two nits and sr=darin
Attachment #188666 -
Flags: superreview?(darin) → superreview+
Assignee | ||
Comment 8•19 years ago
|
||
(In reply to comment #7) > (From update of attachment 188666 [details] [diff] [review] [edit]) > nit: indentation is being changed by this patch. i think you > should stick to the indentation style of the existing file. I've tried to match indentation of the file section I was working on for each hunk.. your request brings us to the obvious question, however: what _is_ the indentation style of the file? It seems to be a mix of 2, 4 and an arbitrary number of spaces. The prevalent indentation seems to be 4-spaces, though. I'll assume that's what you want me to use.
Assignee | ||
Comment 9•19 years ago
|
||
Attachment #188666 -
Attachment is obsolete: true
Attachment #189099 -
Flags: approval1.8b4?
Updated•19 years ago
|
Attachment #189099 -
Flags: approval1.8b4? → approval1.8b4+
Assignee | ||
Comment 10•19 years ago
|
||
Checked in by timeless (2005-07-13 12:31).
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Crash Signature: [@ GetSlotWithMechanism]
[@ nsKeygenFormProcessor::GetPublicKey]
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•