110665 - Leak imap password if it's blank

Status: VERIFIED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[David :Bienvenu]

If we don't have an imap password yet, we leak the one byte string we get back
from ToNewCString (probably a result of jag's string changes). Fix (to use
nsXPIDLCString so we'll be less vulnerable to people messing up our code with
string changes) coming up.

Comment 1

[David :Bienvenu]

Attachment: proposed fix

Comment 2

[Navin Gupta]

why did you remove all the null-checks? I know because I have fixed 
crashes which use similar constructs, please check for null.

Comment 3

[David :Bienvenu]

Navin, an automatic nsXPIDLCString can't be null.

Comment 4

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

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

sr=sspitzer

Comment 5

[David :Bienvenu]

Attachment: changed to use IsEmpty

I changed this to use IsEmpty instead of looking at the length. I tried it with
an empty password and didn't crash. Navin, could you elaborate more on why you
feel null checks are needed?

Comment 6

[Navin Gupta]

ok, r=naving

Comment 7

[Navin Gupta]

They keep changing strings. A couple of months back I was made to check for 
null for pop3 GetPassword. 

Comment 8

[David :Bienvenu]

Yeah, it's a pain the way they keep changing the string classes. Are you
referring to this code? :

        nsXPIDLString passwordTitle;
        mStringService->GetStringByID(POP3_ENTER_PASSWORD_PROMPT_TITLE,
getter_Copies(passwordTitle));
        if (passwordPromptString)
        {
          if (passwordTitle)
            rv =  server->GetPasswordWithUI(passwordPromptString,
passwordTitle.get(),
                                          aMsgWindow, okayValue, aPassword);
          nsTextFormatter::smprintf_free(passwordPromptString);
        }


this looks wrong to me - I think you should be checking
!passwordTitle.IsEmpty(). Are they equivalent, Seth?

Comment 9

[Navin Gupta]

yes, i was referring to this code.
if (passwordTitle) checks if PRUnichar* is null, which i believe is the same 
thing as IsEmpty --> Length() == 0

Comment 10

[David :Bienvenu]

fix checked in.

Comment 11

[Karen Huang]

Stephen, can you help for verifying this bug? Thanks. :-)

Comment 12

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

This is VERIFIED FIXED, but check out bug 99140 for some window-leaking 
nastiness!