283569 - OOM crash [@ nsNSS_SSLGetClientAuthData]

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect)

Description

[timeless]

checking only some allocations isn't enough :)

the first alloc is negotiable, in my mind, giving the wrong result (null) when
there's supposed to be a real string because you ran out of memory is the wrong
thing to do, i'd rather propogate an exception.

Comment 1

[Bastiaan Jacques]

Attachment: fix

Comment 2

[Bastiaan Jacques]

Attachment: same as before + indentation fix

Comment 3

[Bastiaan Jacques]

Attachment: same + yet another style fix

I wish I would notice these things earlier..

Comment 4

[Kai Engert (:KaiE:)]

Comment on attachment 189286 [details] [diff] [review]
same + yet another style fix

I'm mostly ok with the patch, however.

>     certNicknameList = (PRUnichar **)nsMemory::Alloc(sizeof(PRUnichar *) * nicknames->numnicknames);
>+    if (!certNicknameList)
>+      goto loser;
>     certDetailsList = (PRUnichar **)nsMemory::Alloc(sizeof(PRUnichar *) * nicknames->numnicknames);
>+    if (!certDetailsList) {
>+      nsMemory::Free(certNicknameList);
>+      goto loser;
>+    }

I think these allocations do no zero initialize the array of pointers?


>-        if (NS_SUCCEEDED(tempCert->FormatUIStrings(i_nickname, nickWithSerial, details))) {
>-          certNicknameList[CertsToUse] = ToNewUnicode(nickWithSerial);
>-          certDetailsList[CertsToUse] = ToNewUnicode(details);
>-        }
>-        else {
>-          certNicknameList[CertsToUse] = nsnull;
>-          certDetailsList[CertsToUse] = nsnull;


This old code inside the loop ensures that each entry will either be NULL or a
valid pointer.


>+      certNicknameList[CertsToUse] = ToNewUnicode(nickWithSerial);
>+      if (!certNicknameList[CertsToUse])
>+        continue;
>+      certDetailsList[CertsToUse] = ToNewUnicode(details);
>+      if (!certDetailsList[CertsToUse]) {
>+        nsMemory::Free(certNicknameList[CertsToUse]);
>+        continue;


You no longer do that in your new code.
You even destroy objects, and continue to have the pointer.
The free_array calls later in the patch will try to free the dangling pointer
and crash.

And we even continue to loop, although we ran into the OOM.


I have two suggestions:

Suggestion 1)

Directly after the allocation, make sure everything is being set to zero.
Then we could break on the failure, see XXX:

>+      certNicknameList[CertsToUse] = ToNewUnicode(nickWithSerial);
>+      if (!certNicknameList[CertsToUse])
>+        break; // XXX
>+      certDetailsList[CertsToUse] = ToNewUnicode(details);
>+      if (!certDetailsList[CertsToUse]) {
>+        nsMemory::Free(certNicknameList[CertsToUse]);
>+        break; // XXX


Suggestion 2)

If you don't want the initial setting to zero, we could just go on after the
OOM, but you'd have to ensure you set everything to zero inside the loop.

>+      certDetailsList[CertsToUse] = nsnull;
>+      certNicknameList[CertsToUse] = ToNewUnicode(nickWithSerial);
>+      if (!certNicknameList[CertsToUse])
>+        continue;
>+      certDetailsList[CertsToUse] = ToNewUnicode(details);
>+      if (!certDetailsList[CertsToUse]) {
>+        nsMemory::Free(certNicknameList[CertsToUse]);
>+        certNicknameList[CertsToUse] = nsnull;
>+        continue;


I think Suggestion 1 is simpler.

Comment 5

[Bastiaan Jacques]

(In reply to comment #4)
[snip]
> I think these allocations do no zero initialize the array of pointers?

I think you mean you're not sure if nsMemory::Alloc() returns NULL on failure
for pointer arrays. It does in fact do that, the pointer type has no effect on
it. Note that Alloc() isn't even "aware" of the pointer type: it is only passed
a number in its argument.

So I'm simply checking to make sure that certNicknameList is a valid pointer, so
it's safe to use it later. Whether the pointer points to a 1-, 2- or
3-dimensional array is irrelevant.

[snip]
> This old code inside the loop ensures that each entry will either be NULL or a
> valid pointer.
[snip]
> You no longer do that in your new code.

And I don't have to, because when the arrays are used (in
nsNSSDialogs::ChooseCertificate()), the paramount array indicator is CertsToUse.
So if an alloc should fail, CertsToUse isn't increased (because the for loop is
|continue|d) and nsNSSDialogs::ChooseCertificate() will not use the invalid part
of the array for which we failed to allocate memory.

> You even destroy objects, and continue to have the pointer.
> The free_array calls later in the patch will try to free the dangling pointer
> and crash.

My above comment about nsNSSDialogs::ChooseCertificate() applies to the
NS_FREE_XPCOM_ALLOCATED_POINTER_ARRAY macro as well; CertsToUse determines how
many array entries are to be freed. In fact, the macro works the same way the
old code did, i.e.:

for (i = 0; i < CertsToUse; ++i) { }

and certNicknameList[i] is always a valid pointer, because when we fail to
allocate memory, we don't increment CertsToUse.

> And we even continue to loop, although we ran into the OOM.

Indeed. But since we can recover from the OOM error (with an partial but _valid_
pointer array), I don't think it's a bad idea to do so. Breaking off immediately
results in a silent failure (or worse).

[snip]
> Suggestion 1)
> 
> Directly after the allocation, make sure everything is being set to zero.
> Then we could break on the failure, see [snip]

I think it's better to continue, as I mentioned.

> Suggestion 2)
> 
> If you don't want the initial setting to zero, we could just go on after the
> OOM, but you'd have to ensure you set everything to zero inside the loop.

This isn't needed, as long as CertsToUse maintains an accurate count of the
number of pointer arrays (I described this in earlier in this comment).

Comment 6

[Bastiaan Jacques]

CC'ing Kai, so he will hopefully read my response.

Comment 7

[Kai Engert (:KaiE:)]

In short, you've concinved me, but I want to reply in detail:


(In reply to comment #5)
> (In reply to comment #4)
> [snip]
> > I think these allocations do no zero initialize the array of pointers?
> 
> I think you mean you're not sure if nsMemory::Alloc() returns NULL on failure
> for pointer arrays. 

No, I meant it does not init what's INSIDE the memory block, which we are using
as an array of pointers, and therefore it contains random pointers initially.


Thanks for making me aware that CertsToUse controls how many items will be
destroyed. I now agree that your code is correct.

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 189286 [details] [diff] [review]
same + yet another style fix

r=kaie

(and restoring your request to ask darin for sr)

Comment 9

[Darin Fisher]

Comment on attachment 189286 [details] [diff] [review]
same + yet another style fix

>Index: security/manager/ssl/src/nsNSSIOLayer.cpp

>+      nsAutoString i_nickname(NS_ConvertUTF8toUCS2(nicknames->nicknames[CertsToUse]));

This could be simplified down to the following, which avoids a string copy:

	NS_ConvertUTF8toUTF16 i_nickname(nicknames->nicknames[CertsToUse]);


sr=darin with that change

Comment 10

[Bastiaan Jacques]

Attachment: patch for checkin

Addresses Darin's comment.

Comment 11

[Bastiaan Jacques]

Checked in by timeless (2005-08-02 07:22).