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

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

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).

Comment 1

[Bastiaan Jacques]

Attachment: fix

Comment 2

[Bastiaan Jacques]

Attachment: better fix

Comment 3

[Bastiaan Jacques]

Attachment: EVEN better fix ;)

Comment 4

[Kai Engert [: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.

Comment 5

[Bastiaan Jacques]

Attachment: address comments

Comment 6

[Kai Engert [:KaiE:]]

Comment on attachment 188666 [details] [diff] [review]
address comments

r=kaie

Comment 7

[Darin Fisher]

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

Comment 8

[Bastiaan Jacques]

(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.

Comment 9

[Bastiaan Jacques]

Attachment: patch for checkin

Comment 10

[Bastiaan Jacques]

Checked in by timeless (2005-07-13 12:31).