201367 - Delete array as if a pointer (memory leak)

Status: VERIFIED FIXED

(MailNews Core :: Attachments, defect)

Description

[Mark Stankus]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.2) Gecko/20030208 Netscape/7.02
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.0.2) Gecko/20030208 Netscape/7.02

A "classic" or "traditional" memory leak found. Are there more like this?

Reproducible: Always

Steps to Reproduce:
1.Look at the code specified below!



mozilla/mailnews/base/src/nsMessenger.cpp has the following code.
Part of the code acts as though the pointers correspond to arrays (of
pointers), but the delete commands delete the elements not pointed too
rather than the array.

nsnsSaveAllAttachmentsState::~nsSaveAllAttachmentsState()
{
    PRUint32 i;
    for (i = 0; i < m_count; i++)
    {
        nsCRT::free(m_contentTypeArray[i]);
        nsCRT::free(m_urlArray[i]);
        nsCRT::free(m_displayNameArray[i]);
        nsCRT::free(m_messageUriArray[i]);
    }
    delete m_contentTypeArray;
    delete m_urlArray;
    delete m_displayNameArray;
    delete m_messageUriArray;
    nsCRT::free(m_directoryName);
}

A "classic" memory leak.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Not a browser bug..  But yes, chances are there are more like this.  ;)

Comment 2

[Christian :Biesinger (don't email me, ping me on IRC)]

maybe nsAutoArrayPtr should be used here

Comment 3

[Stephen Moehle]

I do not see a real leak here.  The deletes should be written as "delete[]", but
since the arrays are arrays of char ptrs, there are no destructors to run for
the array elements, it is not going to make any real difference.  At least for
most compilers, anyhow.

Comment 4

[Boris Zbarsky [:bzbarsky]]

ccing someone who knows enough c++ to tell whether this is a leak... and
hopefully reads bugmail.

Comment 5

[Bradley Baetz (:bbaetz)]

it needs to be delete[] so that more than the first element is deleted (well,
depending on the ABI and all that. ISTR that its unspecified what |delete| does
to something allocated via |new[]|)

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: patch

Comment 7

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

Comment on attachment 121097 [details] [diff] [review]
patch

sr=sspitzer

Comment 8

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

re-assigning to [e-mail]

Comment 9

[Boris Zbarsky [:bzbarsky]]

Checked in.  Thanks for pointing out the problem, Mark!

Comment 10

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified via code-inspection using LXR:

1.265	bzbarsky%mit.edu	Apr 22 10:42	 	Use delete[] for arrays, not delete. Bug
201367, patch by [e-mail]
(Mark Stankus), r=bbaetz, sr=sspitzer