110700 - MLK: Memory leak of 1 byte from 1 block allocated in PL_strdup

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect)

Description

[Navin Gupta]

Distribution of leaked blocks
        Allocation location
            malloc         [msvcrt.DLL]
            PL_strdup      [strdup.c:46]
            nsPrefService::GetCharPref(char const*,char * *) 
[nsPrefService.h:57]
            MimeInlineTextPlain_parse_begin [mimetpla.cpp:165]
            MimeMessage_close_headers [mimemsg.cpp:457]
            MimeMessage_parse_line [mimemsg.cpp:271]
            convert_and_send_buffer [mimebuf.cpp:168]
            mime_LineBuffer [mimebuf.cpp:255]
            MimeObject_parse_buffer [mimeobj.cpp:255]
            nsStreamConverter::OnDataAvailable(nsIRequest *,nsISupports 
*,nsIInputStream *,UINT,UINT) [nsStreamConverter.cpp:919]
            nsDocumentOpenInfo::OnDataAvailable(nsIRequest *,nsISupports 
*,nsIInputStream *,UINT,UINT) [nsURILoader.cpp:240]
            nsMailboxProtocol::ReadMessageResponse(nsIInputStream *,UINT,UINT) 
[nsMailboxProtocol.cpp:570]
            nsMailboxProtocol::ProcessProtocolState(nsIURI *,nsIInputStream 
*,UINT,UINT) [nsMailboxProtocol.cpp:661]
            nsMsgProtocol::OnDataAvailable(nsIRequest *,nsISupports 
*,nsIInputStream *,UINT,UINT) [nsMsgProtocol.cpp:262]
            nsOnDataAvailableEvent::HandleEvent(void) 
[nsStreamListenerProxy.cpp:193]
            PL_HandleEvent [plevent.c:590]
            md_EventReceiverProc [plevent.c:1071]
            ScrollDC       [user32.dll]
            ScrollDC       [user32.dll]
            DispatchMessageA [user32.dll]

Comment 1

[Jean-Francois Ducarroz]

we are leaking mCitationColor. Apparently an empty string in that case but could
be  more.

Comment 2

[Jean-Francois Ducarroz]

We need to do the same than in
MimeInlineTextPlainFlowed::MimeInlineTextPlainFlowed_parse_eof. Patch coming...

Comment 3

[Jean-Francois Ducarroz]

I cannot generate a diff right now because of a cvs lock :-(

Comment 4

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v1

Comment 5

[Navin Gupta]

Comment on attachment 60394 [details] [diff] [review]
Proposed fix, v1

looks ok to me.

Comment 6

[varada]

Comment on attachment 60394 [details] [diff] [review]
Proposed fix, v1

r=varada.
We should file a bug and get rid of these gotos in mime though :)

Comment 7

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

instead of doing it this way, could we make mCitation color an nsXPIDLCString or
a nsCString, so that when the text object gets destroyed, we'll clean up after
ourselves automatically?

Comment 8

[Jean-Francois Ducarroz]

yes I can but that would be a revolution in Mine to use anything else than a
char* :-)

Comment 9

[Scott MacGregor]

revolutions need to start somewhere =).

Comment 10

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v2

I've replaced "char *mCitationColor" by "nsXPIDLCString mCitationColor"

Comment 11

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

Comment on attachment 61124 [details] [diff] [review]
Proposed fix, v2

Viva la Revolucion!  

one question, where we used to free the mCitation color, should we be setting
it to null?

Comment 12

[Jean-Francois Ducarroz]

no. mCitationColor is used only in this file and we set it to an empty string
every time we start using it. Therefore setting it to null when done with is
wasn't needed.

Comment 13

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

Comment on attachment 61124 [details] [diff] [review]
Proposed fix, v2

thanks for the explanation.  r=sspitzer

Comment 14

[Jean-Francois Ducarroz]

Comment on attachment 61124 [details] [diff] [review]
Proposed fix, v2

This patch does not work!

Comment 15

[Jean-Francois Ducarroz]

The second patch does not work because mine create new mime class using MALLOC
and not new. Therefore you cannot use an XPIDLString, that will cause a crash!

Seth, can you SR the first patch. Thanks.

Comment 16

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

is there any string trickery would could here, to avoid the goto?

like adopting the text->mCitationColor after this line:

if (obj->closed_p) return 0;

nsCAutoString citationColor;
citationColor.Adopt(text->mCitationColor);

and then when the method returns, and the adopted parent of text-
>mCitationColor goes out of scope (as it would be a stack based object), it 
would free the buffer?

cc jag / dbaron, they've got string string kung foo.

Comment 17

[Jean-Francois Ducarroz]

Attachment: Proposed fix, v3

Comment 18

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

Comment on attachment 61167 [details] [diff] [review]
Proposed fix, v3

sr=sspitzer

looks good.  this is a trick we could use to fix some other confusing goto in
mailnews.

Comment 19

[Jean-Francois Ducarroz]

Fixed and checked in

Comment 20

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

Verified FIXED, using my vcard test message with inline text, running on 
Windows 2000 under Purify with the latest trunk build as of 11:35 pm December 
10th.

Comment 21

[jag (Peter Annema)]

Yep, this is what I would have suggested.