253394 - nsFtpState::S_user, S_pass leak user & password strings

Status: VERIFIED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

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

PRUnichar *user = nsnull, *passwd = nsnull;
...
          rv = mAuthPrompter->PromptUsernameAndPassword(nsnull,
                                                          formatedString,
                                                          prePathU.get(),
                                                         
nsIAuthPrompt::SAVE_PASSWORD_PERMANENTLY,
                                                          &user, 
                                                          &passwd, 
                                                          &retval);


There is never a nsMemory::Free call for user and passwd. This should probably
use nsXPIDLString, or use .Adopt to assign to mUser and mPassword, or make sure
to call nsMemory::Free when needed.

Comment 1

[Doug Turner (:dougt)]

Index: nsFtpConnectionThread.cpp
===================================================================
RCS file: /cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v
retrieving revision 1.283
diff -u -r1.283 nsFtpConnectionThread.cpp
--- nsFtpConnectionThread.cpp	29 Jun 2004 16:45:05 -0000	1.283
+++ nsFtpConnectionThread.cpp	29 Jul 2004 14:07:33 -0000
@@ -1007,8 +1007,8 @@
             // if the user canceled or didn't supply a username we want to fail
             if (!retval || (user && !*user) )
                 return NS_ERROR_FAILURE;
-            mUsername = user;
-            mPassword = passwd;
+            mUsername.Adopt(user);
+            mPassword.Adopt(passwd);
         }
         usernameStr.AppendWithConversion(mUsername);    
     }

Comment 2

[Kathleen :Brade]

r=brade

Comment 3

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

             if (!retval || (user && !*user) )
                 return NS_ERROR_FAILURE;

won't this return still leak the password, and the user if it is an empty
non-null string?

Comment 4

[jon rekai]

That sounds right..

Maybe looking to 

> use .Adopt to assign to mUser and mPassword, 
AND 
> make sure
> to call nsMemory::Free when needed.

Comment 5

[jon rekai]

Attachment: no leaky me

Comments 1 and 3/4.  (Also good for 1.7, and s/b for aviary, too.)

Comment 6

[Michiel van Leeuwen (email: mvl+moz@)]

nsXPIDLString does all that freeing automaticly. Why not use it? I think the
code will become more readable.

Comment 7

[jon rekai]

Attachment: fix using nsXPIDLString

String are s/scary/my friend/.

Would the (user, passwd, maybe retval) declarations fit better immediately
preceeding PromptUsernameAndPassword()?   (user, passwd because that line's
changing anyway, and retval, too, for consistency.)

Comment 8

[Michiel van Leeuwen (email: mvl+moz@)]

usually, moving will be better. But look at the surrounding code first. What is
the convention in the ftp code?

Comment 9

[jon rekai]

Comment on attachment 156736 [details] [diff] [review]
fix using nsXPIDLString

Moving the declaration shouldn't break convention.  (However, will let the r?
call it.)

Comment 10

[Mikael Parknert]

nsFtpState::S_pass()
also leaks passwd

http://lxr.mozilla.org/seamonkey/source/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp#1110

Comment 11

[jon rekai]

(In reply to comment #10)
> nsFtpState::S_pass()
> also leaks passwd

Looks like a similar issue, and can probably be treated in a like manner.  Maybe
the scope of this bug should be expanded to address this also?

Comment 12

[Mikael Parknert]

(In reply to comment #11)
> Looks like a similar issue, and can probably be treated in a like manner.  Maybe
> the scope of this bug should be expanded to address this also?

Probably a good idea :-)

Comment 13

[jon rekai]

Attachment: S_pass(), too

Comment 14

[Mikael Parknert]

John, if you wan't this patch into Necko, please ask for reviews

Comment 15

[Darin Fisher]

Comment on attachment 163991 [details] [diff] [review]
S_pass(), too

r+sr=darin

Comment 16

[Darin Fisher]

fixed-on-trunk

thanks for the patch!

Comment 17

[jon rekai]

v.

Thanks!