Memory leak of 169 bytes from 1 block allocated in WriteLineToStream

VERIFIED FIXED

Status

MailNews Core
MIME
--
major
VERIFIED FIXED
16 years ago
10 years ago

People

(Reporter: stephend@netscape.com (gone - use stephen.donner@gmail.com instead), Assigned: Navin Gupta)

Tracking

({mlk})

Trunk
x86
Windows 2000

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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 );
                }
Created attachment 59824 [details]
Message which shows this leak.
Keywords: mlk
QA Contact: esther → stephend
Blocks: 110171
(Assignee)

Comment 2

16 years ago
taking
Assignee: ducarroz → naving
(Assignee)

Comment 3

16 years ago
Created attachment 60117 [details] [diff] [review]
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

16 years ago
Er, I don't think we have any 169 byte charsets. Are you sure that's what the
leak is?
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
Attachment #60117 - Flags: review+
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

16 years ago
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.
Attachment #60117 - Flags: superreview+
(Assignee)

Comment 9

16 years ago
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. 
(Assignee)

Comment 10

16 years ago
why do i need to change it to PR_FREEIF, it never points to garbage..
(Assignee)

Comment 11

16 years ago
I meant it points to null or valid memory.
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

16 years ago
PR_Free crashes if you pass it null.

Comment 14

16 years ago
Or, rather, you should assume it does.

Comment 15

16 years ago
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.
(Assignee)

Comment 16

16 years ago
fixed
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Nice fix, this is indeed FIXED with the latest trunk Win32 pull, Purify.
Status: RESOLVED → VERIFIED
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.