OOM crash [@ nsNSS_SSLGetClientAuthData]

RESOLVED FIXED

Status

Core Graveyard
Security: UI
RESOLVED FIXED
14 years ago
2 years ago

People

(Reporter: timeless, Assigned: Bastiaan Jacques)

Tracking

({crash})

Other Branch
x86
Windows XP
crash

Firefox Tracking Flags

(Not tracked)

Details

(crash signature, URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

14 years ago
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.
(Reporter)

Updated

14 years ago
Component: Security: UI → Security: UI
Product: PSM → Core
(Assignee)

Comment 1

13 years ago
Created attachment 189201 [details] [diff] [review]
fix
Assignee: kaie.bugs → b.jacques
Status: NEW → ASSIGNED
Attachment #189201 - Flags: review?(kaie.bugs)
(Assignee)

Comment 2

13 years ago
Created attachment 189285 [details] [diff] [review]
same as before + indentation fix
Attachment #189201 - Attachment is obsolete: true
Attachment #189285 - Flags: superreview?(darin)
Attachment #189285 - Flags: review?(kaie.bugs)
(Assignee)

Updated

13 years ago
Attachment #189201 - Flags: review?(kaie.bugs)
(Assignee)

Comment 3

13 years ago
Created attachment 189286 [details] [diff] [review]
same + yet another style fix

I wish I would notice these things earlier..
Attachment #189285 - Attachment is obsolete: true
Attachment #189286 - Flags: superreview?(darin)
Attachment #189286 - Flags: review?(kaie.bugs)
(Assignee)

Updated

13 years ago
Attachment #189285 - Flags: superreview?(darin)
Attachment #189285 - Flags: review?(kaie.bugs)

Comment 4

13 years ago
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.
Attachment #189286 - Flags: superreview?(darin)
Attachment #189286 - Flags: review?(kaie.bugs)
Attachment #189286 - Flags: review-
(Assignee)

Comment 5

13 years ago
(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).
(Assignee)

Comment 6

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

Comment 7

13 years ago
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

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

r=kaie

(and restoring your request to ask darin for sr)
Attachment #189286 - Flags: superreview?(darin)
Attachment #189286 - Flags: review-
Attachment #189286 - Flags: review+

Comment 9

13 years ago
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
Attachment #189286 - Flags: superreview?(darin) → superreview+
(Assignee)

Comment 10

13 years ago
Created attachment 191326 [details] [diff] [review]
patch for checkin

Addresses Darin's comment.
Attachment #189286 - Attachment is obsolete: true
Attachment #191326 - Flags: approval1.8b4?

Updated

13 years ago
Attachment #191326 - Flags: approval1.8b4? → approval1.8b4+
(Assignee)

Comment 11

13 years ago
Checked in by timeless (2005-08-02 07:22).
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Crash Signature: [@ nsNSS_SSLGetClientAuthData]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.