Closed Bug 283563 Opened 20 years ago Closed 19 years ago

OOM crash [@ GetSlotWithMechanism][@ nsKeygenFormProcessor::GetPublicKey]

Categories

(Core Graveyard :: Security: UI, defect)

Other Branch
x86
Windows XP
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: timeless, Assigned: bastiaan)

References

()

Details

(Keywords: crash, memory-leak)

Crash Data

Attachments

(1 file, 4 obsolete files)

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).
Product: PSM → Core
Attached patch fix (obsolete) — Splinter Review
Assignee: kaie → b.jacques
Status: NEW → ASSIGNED
Attached patch better fix (obsolete) — Splinter Review
Attachment #188485 - Attachment is obsolete: true
Attachment #188493 - Flags: review?(kaie)
Attached patch EVEN better fix ;) (obsolete) — Splinter Review
Attachment #188493 - Attachment is obsolete: true
Attachment #188497 - Flags: review?(kaie)
Attachment #188493 - Flags: review?(kaie)
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-
Attached patch address comments (obsolete) — Splinter Review
Attachment #188497 - Attachment is obsolete: true
Attachment #188666 - Flags: review?(kaie)
Comment on attachment 188666 [details] [diff] [review]
address comments

r=kaie
Attachment #188666 - Flags: review?(kaie) → review+
Attachment #188666 - Flags: superreview?(darin)
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+
(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.
Attachment #188666 - Attachment is obsolete: true
Attachment #189099 - Flags: approval1.8b4?
Attachment #189099 - Flags: approval1.8b4? → approval1.8b4+
Checked in by timeless (2005-07-13 12:31).
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Crash Signature: [@ GetSlotWithMechanism] [@ nsKeygenFormProcessor::GetPublicKey]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: