157184 - Freeing mismatched memory in delete

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[[e-mail] (gone - use [e-mail] instead)]

While verifying bug 156967 (stand-alone window can cause memory corruption), I
found this bug.

To reproduce, just double-click an IMAP message into a stand-alone window.

[E] FMM: Freeing mismatched memory in delete(void *) {1 occurrence}
    Address 0x0a013ec8 points into a malloc'd block in heap 0x02720000
    Location of free attempt
    delete(void *) [MSVCRT.DLL]
    nsUint8Array::SetSize(int,int) [nsUint8Array.cpp:115]
    
            memset(&pNewData[m_nSize], 0, (nNewSize-m_nSize) * sizeof(PRUint8));
    
 =>         delete[] (PRUint8*)m_pData;
            m_pData = pNewData;
            m_nSize = nNewSize;
            m_nMaxSize = nNewMax;
    nsUint8Array::InsertAt(int,BYTE,int) [nsUint8Array.cpp:172]
        if (nIndex >= m_nSize)
        {
            // adding after the end of the array
 =>         SetSize(nIndex + nCount);  // grow so nIndex is valid
        }
        else
        {
    nsMsgDBView::AddHdr(nsIMsgDBHdr *) [nsMsgDBView.cpp:3742]
    nsMsgThreadedDBView::OnNewHeader(UINT,UINT,int) [nsMsgThreadedDBView.cpp:574]
    nsMsgDBView::OnKeyAdded(UINT,UINT,int,nsIDBChangeListener *)
[nsMsgDBView.cpp:4019]
    nsMsgDatabase::NotifyKeyAddedAll(UINT,UINT,int,nsIDBChangeListener *)
[nsMsgDatabase.cpp:538]
    nsMsgDatabase::AddNewHdrToDB(nsIMsgDBHdr *,int) [nsMsgDatabase.cpp:2747]
    nsImapMailFolder::NormalEndHeaderParseStream(nsIImapProtocol *)
[nsImapMailFolder.cpp:2610]
    XPTC_InvokeByIndex [xptcinvoke.cpp:105]
    Allocation location
    malloc         [MSVCRT.DLL]
    PR_Malloc      [prmem.c:470]
    nsUint8Array::CopyArray(nsUint8Array&) [nsUint8Array.cpp:148]
    nsMsgDBView::CopyDBView(nsMsgDBView *,nsIMessenger *,nsIMsgWindow
*,nsIMsgDBViewCommandUpdater *) [nsMsgDBView.cpp:5216]
    nsMsgThreadedDBView::CloneDBView(nsIMessenger *,nsIMsgWindow
*,nsIMsgDBViewCommandUpdater *,nsIMsgDBView * *) [nsMsgThreadedDBView.cpp:884]
    XPTC_InvokeByIndex [xptcinvoke.cpp:105]
    XPCWrappedNative::CallMethod(XPCCallContext&,CallMode::XPCWrappedNative)
[xpcwrappednative.cpp:1994]
    XPC_WN_CallMethod(JSContext *,JSObject *,UINT,long *,long *)
[xpcwrappednativejsops.cpp:1266]
    js_Invoke      [jsinterp.c:788]
    js_Interpret   [jsinterp.c:2743]
    js_Invoke      [jsinterp.c:805]
    js_InternalInvoke [jsinterp.c:880]
    JS_CallFunctionValue [jsapi.c:3428]
    nsJSContext::CallEventHandler(void *,void *,UINT,void *,int *,int)
[nsJSEnvironment.cpp:1042]
    nsJSEventListener::HandleEvent(nsIDOMEvent *) [nsJSEventListener.cpp:182]
    nsEventListenerManager::HandleEventSubType(nsListenerStruct *,nsIDOMEvent
*,nsIDOMEventTarget *,UINT,UINT) [nsEventListenerManager.cpp:1221]
    nsEventListenerManager::HandleEvent(nsIPresContext *,nsEvent *,nsIDOMEvent *
*,nsIDOMEventTarget *,UINT,nsEventStatus *) [nsEventListenerManager.cpp:1901]
    GlobalWindowImpl::HandleDOMEvent(nsIPresContext *,nsEvent *,nsIDOMEvent *
*,UINT,nsEventStatus *) [nsGlobalWindow.cpp:762]
    DocumentViewerImpl::LoadComplete(UINT) [nsDocumentViewer.cpp:1532]
    nsDocShell::EndPageLoad(nsIWebProgress *,nsIChannel *,UINT)
[nsDocShell.cpp:4110]

Comment 1

[Antonio.Xu]

Attachment: patch version 1.00,please r=? & sr=?

I think we should using uniform mode of memory used with nsUInt32Array.cpp
, and we should avoid mismatched mode of memory used,although it is harmless.
please r=? & sr=?

Comment 2

[[e-mail] (gone - use [e-mail] instead)]

Antonio - you should mail review requests.

Comment 3

[David :Bienvenu]

can you get someone else to review and I'll sr? (don't get Seth - we would
rather avoid having super reviewers do reviews)

Comment 4

[Navin Gupta]

Comment on attachment 91315 [details] [diff] [review]
patch version 1.00,please r=? & sr=?

looks good. r=naving

Comment 5

[David :Bienvenu]

Comment on attachment 91315 [details] [diff] [review]
patch version 1.00,please r=? & sr=?

sr=bienvenu

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 91315 [details] [diff] [review]
patch version 1.00,please r=? & sr=?

The smaller patch would have changed only nsUint8Array::CopyArray to avoid
PR_FREEIF and PR_Malloc in favor of delete and new -- why wasn't that done?

/be

Comment 7

[Antonio.Xu]

I like to keeping consistency with nsUint32Array.cpp, It using PR_FREEIF and 
PR_Malloc in nsUint32Array.cpp. So I think using PR_FREEIF and PR_Malloc is the 
best choice instead of new and delete in this case.

Comment 8

[Scott Collins]

Comment on attachment 91315 [details] [diff] [review]
patch version 1.00,please r=? & sr=?

I don't see any functional reason to prefer |PR_Malloc| and |PR_Free| over |new
[]| and |delete []|.  All things being equal, in my own code I would use the
built-ins.  Something to consider for next time.

Comment 9

[Scott Collins]

sorry, should have noted a=scc (on behalf of drivers) for checkin to the mozilla
trunk

Comment 10

[Pete Zha]

checked in

Comment 11

[[e-mail] (gone - use [e-mail] instead)]

Verified FIXED with Purify and the latest trunk pull on Windows 2000.