281093 - OOM check is missing [@nsConsoleService::nsConsoleService]

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Daniel de Wildt]

In the nsConsoleService.cpp constructor is memory allocate but the result is not
checked, which results in a crash when initializing the memory.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Sounds like this should use an Init() method... Daniel, do you have time to work
on this by any chance?

Comment 2

[Daniel de Wildt]

Attachment: Move memory allocation and initialization to new function Init

I suppose that the destructor is called if Init fails, so I don't free the
allocated memory of mMessages if PR_NewLock() fails. 
Who is the correct person for a r/sr. bz or dougt?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175842 [details] [diff] [review]
Move memory allocation and initialization to new function Init

I can probably do this review...

>Index: mozilla/xpcom/base/nsConsoleService.h
>+    NS_IMETHODIMP Init();

That should just be |nsresult|, not |NS_IMETHODIMP|.

>Index: mozilla/xpcom/base/nsConsoleService.cpp
> nsConsoleService::nsConsoleService()
>     : mCurrent(0), mFull(PR_FALSE), mListening(PR_FALSE), mLock(nsnull)

This needs to init mMessages to nsnull, no?

>+NS_IMETHODIMP
>+nsConsoleService::Init()

Again, nsresult.

>+    // XXX deal with these two allocations by detecting null mLock in factory?

This comment no longer applies, does it?

>+    // Array elements should be 0 initially for circular buffer algorithm.
>+    for (PRUint32 i = 0; i < mBufferSize; i++) {
>+        mMessages[i] = nsnull;
>+    }

Any reason not to use memset here?

>+    mLock = PR_NewLock();
>+    if (!mLock)
>+        return NS_ERROR_FAILURE;

Why not NS_ERROR_OUT_OF_MEMORY?

Comment 4

[Daniel de Wildt]

Attachment: Patch #2 (fixes per comment 3)

> This needs to init mMessages to nsnull, no?
yes

> This comment no longer applies, does it?
> Any reason not to use memset here?
Sorry, too much cut&paste

> Why not NS_ERROR_OUT_OF_MEMORY?
I thought PR_NewLock could also return null
in other cases as OOM, but I'm wrong.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 175960 [details] [diff] [review]
Patch #2 (fixes per comment  3)

r+sr=bzbarsky.	I'll try to get this checked in tomorrow...

Comment 6

[Boris Zbarsky [:bzbarsky]]

Patch checked in.  Thanks for fixing this, Daniel!

Comment 7

[Daniel de Wildt]

Marking fixed