156967 - using stand-alone msg window can cause memory corruption

Status: VERIFIED FIXED

(MailNews Core :: Backend, defect)

Description

[David :Bienvenu]

The trunk only performance ehancement to clone msg views when opening
stand-alone msg windows  instead of creating a new view has exposed bugs in our
array cloning code. In particular, the clone code was not setting the m_nMaxSize
of the new array correctly, so that the array thought it had more space than it
did (the clone code was allocating space for the actual number of entries the
src array had, but setting the allocated size to the allocated size of the src
array, which is usually larger). This results in memory corruption when adding
elements to the cloned array. I'll attach a patch in a sec.

Comment 1

[David :Bienvenu]

Attachment: proposed fix

Navin, can I get a review? thx. There were two possible ways of fixing this - I
could have changed the allocation size for the new array to the allocated size
of the old array, but that would mean we could allocated more memory than
needed, so I chose the attached fix.

Comment 2

[Navin Gupta]

Comment on attachment 90990 [details] [diff] [review]
proposed fix

ok, makes sense, r=naving 
we could grow as needed.

Comment 3

[David :Bienvenu]

right, we will grow as needed.

Comment 4

[(not reading, please use [e-mail] instead)]

cc [e-mail].

the fix for #109557 caused this bug.

Comment 5

[(not reading, please use [e-mail] instead)]

Comment on attachment 90990 [details] [diff] [review]
proposed fix

sr=sspitzer

Comment 6

[Asa Dotzler [:asa]]

Comment on attachment 90990 [details] [diff] [review]
proposed fix

a=asa (on behalf of drivers) for checkin to the 1.1 trunk.

Comment 7

[David :Bienvenu]

fix checked into trunk.

Comment 8

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

David, I've since updated my tree (doh!), but before you fixed this, did you see
an Array Bounds Read in Purify, or was it some other error?

Comment 9

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

My tree is up-to-date:

C:\moz_src\mozilla\mailnews\base\util>cvs up
cvs server: Updating .
cvs server: Updating macbuild

However, I see something that should be related, or a straight spinoff...

[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 10

[Antonio.Xu]

Attachment: patch version 1.00

I use pr_malloc and pr_free replace new and delete in nsUInt8Array.cpp. I think
it can resolve Stephen's problem. I think we should use pr_malloc instead of
new. If you think we should use "new", we should change some code in
CopyArray() like this

void nsUint8Array::CopyArray(nsUint8Array &aSrcArray)
{
-  PR_FREEIF(m_pData);
+ if (m_pData)
+   delete[] (PRUint8*)m_pData;
  m_nSize = aSrcArray.m_nSize;
  m_nMaxSize = aSrcArray.m_nSize;
-  m_pData = (PRUint8*)PR_Malloc(m_nSize * sizeof(PRUint8));
+  m_pData = (PRUint8*) new PRUint8[m_nSize * sizeof(PRUint8)]; 
  if (m_pData)
   memcpy(m_pData, aSrcArray.m_pData, m_nSize * sizeof(PRUint8));
}

Comment 11

[David :Bienvenu]

Stephen, that's unrelated and harmless. What you'd see in purify is array bounds
write, i.e., writing past the end of the array for this bug.

Comment 12

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

Verified FIXED, David's right, an FMM is less critical than an ABR.
 
I'll file a new bug, no need to continue the new problem here.

Comment 13

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

I've filed bug 157184.  Antonio, please carry your patch to that bug and
continue discussion there.  Thanks!

Comment 14

[Margaret Chan]

Antonio:  According to bonsai, you have already checked this into the OEM
branch, correct?  Marked it as fixedOEM.  Please correct it if it is not right.

Comment 15

[Antonio.Xu]

Sorry for my careless, I have checked the patch into oem branch.
Thanks  Margaret