112783 - Memory leak of 169 bytes from 1 block allocated in WriteLineToStream

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect)

Description

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

Build ID:  Latest trunk Windows 2000 using Purify.

Steps to Reproduce: I'll attach the message that produces this, it's a simple 
HTML message with an VCARD attachment (viewed inline).

[W] MLK: Memory leak of 169 bytes from 1 block allocated in PL_strdup
        Distribution of leaked blocks
        Allocation location
            malloc         [msvcrt.DLL]
            PL_strdup      [strdup.c:46]
            WriteLineToStream [mimevcrd.cpp:1790]
                    // Seek out a charset!
                    charset = PL_strcasestr(obj->content_type, "charset=");
                    if (!charset)
             =>       charset = FindCharacterSet(obj);
                
                    if ( (!charset) || ( (charset) &&
(!nsCRT::strcasecmp(charset, "us-ascii"))) )
                      charset = nsCRT::strdup("ISO-8859-1");
            WriteValue     [mimevcrd.cpp:1841]
                {
                    int status = 0;
                    OutputFont(obj, PR_FALSE, "-1", NULL);
             =>     status  = WriteLineToStream (obj, value, PR_TRUE);
                    OutputFont(obj, PR_TRUE, NULL, NULL);
                    return status;
                }
            WriteOutVCardProperties [mimevcrd.cpp:1701]
                    VObjectIterator t;
                    VObject *eachProp;
                
             =>     WriteOutEachVCardProperty (obj, v, numEmail);
                    initPropIterator(&t,v);
                    while (moreIteration(&t) && status >= 0)
                    {
            WriteOutVCardProperties [mimevcrd.cpp:1706]
                    while (moreIteration(&t) && status >= 0)
                    {
                        eachProp = nextVObject(&t);
             =>         status = WriteOutVCardProperties (obj, eachProp, numEmail);
                    }
                
                    if (status < 0) return status;
            OutputAdvancedVcard [mimevcrd.cpp:972]
                    if (status < 0) return status;
                
                    /* beginning of remaining rows */
             =>     status = WriteOutVCardProperties (obj, v, &numEmail);
                    if (status < 0) return status;
                
                    status = OutputTable (obj, PR_TRUE, PR_FALSE, NULL, NULL, NULL);
            nsStreamListenerEvent0::HandlePLEvent(PLEvent *)
[nsAsyncStreamListener.cpp:113]
                    nsStreamListenerEvent* ev = GET_STREAM_LISTENER_EVENT(aEvent);
                    NS_ASSERTION(nsnull != ev,"null event.");
                
             =>     nsresult rv = ev->HandleEvent();
                    //
                    // If the consumer fails, then cancel the transport.  This
is necessary
                    // in case where the socket transport is blocked waiting for
room in the
            md_EventReceiverProc [plevent.c:1071]
                    {
                        PREventQueue *queue = (PREventQueue *)lParam;
                        queue->removeMsg = PR_FALSE;
             =>         PL_ProcessPendingEvents(queue);
                        queue->removeMsg = PR_TRUE;
                #ifdef XP_OS2
                        return MRFROMLONG(TRUE);
            ScrollDC       [user32.dll]
            ScrollDC       [user32.dll]
            DispatchMessageA [user32.dll]
            DispatchMessageA [USER32.DLL]
            nsAppShell::Run(void) [nsAppShell.cpp:121]
                //      if (!nsToolkit::gAIMMMsgPumpOwner ||
(nsToolkit::gAIMMMsgPumpOwner->OnTranslateMessage(&msg) != S_OK))
                //#endif
                        TranslateMessage(&msg);
             =>         ::DispatchMessage(&msg);
                        if (mDispatchListener)
                          mDispatchListener->AfterDispatch();
                      }
            nsAppShellService::Run(void) [nsAppShellService.cpp:301]
                
                NS_IMETHODIMP
                nsAppShellService::Run(void)
             => {
                  return mAppShell->Run();
                }
                
            main1          [nsAppRunner.cpp:1269]
                
                  // Start main event loop
                  NS_TIMELINE_ENTER("appShell->Run");
             =>   rv = appShell->Run();
                  NS_TIMELINE_LEAVE("appShell->Run");
                  NS_ASSERTION(NS_SUCCEEDED(rv), "failed to run appshell");
                
            main           [nsAppRunner.cpp:1599]
                  }
                #endif
                
             =>   nsresult mainResult = main1(argc, argv, nativeApp ?
(nsISupports*)nativeApp : (nsISupports*)splash);
                
                  /* if main1() didn't succeed, then don't bother trying to shut
down clipboard, etc */
                  if (NS_SUCCEEDED(mainResult)) {
            WinMain        [nsAppRunner.cpp:1617]
                // We need WinMain in order to not be a console app.  This
function is
                // unused if we are a console application.
                int WINAPI WinMain( HINSTANCE, HINSTANCE, LPSTR args, int )
             => {
                    // Do the real work.
                    return main( __argc, __argv );
                }

Comment 1

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

Attachment: Message which shows this leak.

Comment 2

[Navin Gupta]

taking

Comment 3

[Navin Gupta]

Attachment: proposed fix

fix leaking charset before assigning new value. charset is initialized to null
so just PR_free will do it. david and jf, please review.

Comment 4

[David :Bienvenu]

Er, I don't think we have any 169 byte charsets. Are you sure that's what the
leak is?

Comment 5

[Jean-Francois Ducarroz]

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

as charset might be null, you must use PR_FREEIF instead of PR_Free. Apart
that, R=ducarroz

Comment 6

[Navin Gupta]

you can delete null and that variable 'charset' gets allocated multiple times
for just one message - so it can reach 160 bytes.

Comment 7

[David :Bienvenu]

Purify says "Memory leak of 169 bytes from 1 block", which if I understand it
correctly, means 1 169 byte block

Comment 8

[David :Bienvenu]

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

you do need to change this to PR_FREEIF. I'm not convinced this fixes the leak
purify is pointing out, but I believe it does fix a leak.

Comment 9

[Navin Gupta]

you are correct,I realized that the address will be different each time.

ok, if you look at FindCharacterSet it tries to get the charset from the 
header so this tCharSet is allocated a length of close to 200 bytes because
it gets it from cTypePtr which gets it from workstring that is allocated 200 
bytes or something like that. tcharset just does some parsing but leaves the
length as it is therefore it is like 169 bytes. 

Comment 10

[Navin Gupta]

why do i need to change it to PR_FREEIF, it never points to garbage..

Comment 11

[Navin Gupta]

I meant it points to null or valid memory.

Comment 12

[Jean-Francois Ducarroz]

It's much cleaner to use PR_FREEIF when the pointer could be null, you never
know if the implementation of PR_Free will always do the test for you! Else why
do we bother having a PR_FREEIF macro!

Comment 13

[David :Bienvenu]

PR_Free crashes if you pass it null.

Comment 14

[David :Bienvenu]

Or, rather, you should assume it does.

Comment 15

[David :Bienvenu]

OK, I take it back - PR_Free is supposed to have the same semantics as unix
free, and unix free is supposed to handle getting passed a null pointer. So,
PR_Free is fine, as long as no one is using the charset pointer later on in the
function.

Comment 16

[Navin Gupta]

fixed

Comment 17

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

Nice fix, this is indeed FIXED with the latest trunk Win32 pull, Purify.