258234 - Creating a new account with existing name crashes Mozilla

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Jarmo]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040902
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a3) Gecko/20040902

If you try to overwrite an existing account by creating a new account Mozilla
crashes.

Reproducible: Always
Steps to Reproduce:
1. Make sure you have a news account already, like "news.gmane.org".
2. Press "Create a new account" and select "newsgroup account".
3. Fill in bogus (or real) data in identity, and press next.
4. In "Server information" type in "news.gmane.org" and press next.
Actual Results:  
Mozilla crashes.

Expected Results:  
Mozilla should say something like "Hey stupid, that name already exist".



*I* do not found it smart to name the account the same as the server.
There might be users who would want to create several accounts on the same server.
In my case, I was just tired. Still the crash is a bug that should be fixed.

Comment 1

[Andrew Schultz]

confirmed with linux build 20040907
this regressed bewteen linux trunk 2004081806 and 2004081906, implicating bug 41929

Comment 2

[Andrew Schultz]

Attachment: stacktrace

(gdb) frame 7
#7  0x04401471 in nsMsgAccountManager::findServerUrl (aElement=0xa3963d0,
data=0xfef47e74)
    at nsMsgAccountManager.cpp:2084
2084	  if ((!*entry->type || (strcmp(entry->type, thisType)==0)) &&
(gdb) p entry->type
$2 = 0xfef47ebc "nntp"
(gdb) p thisType
$3 = {<nsCString> = {<nsCSubstring> = {<nsACString> = {mVTable = 0x8087f68,
mData = 0xa760690 "nntp", mLength = 4, 
	mFlags = 9}, <No data fields>}, <No data fields>}, <No data fields>}

Comment 3

[David :Bienvenu]

taking; I'll look into it tomorrow.

Comment 4

[Kevin Teuscher]

Attachment: wallpaper

This changes to use nsCRT::strcmp which doesn't crash when checking a news
server like the system version of strcmp.  The crash comes because
entry->username is empty (no usernames for news servers).
In the old method that is still in the file (findServer), we use PL_strcmp. 
See
http://lxr.mozilla.org/seamonkey/source/mailnews/base/src/nsMsgAccountManager.cpp#2130

We can use PL_strcmp or nsCRT::strcmp

This was switched in the new method to the system strcmp per your comments in
bug 41929 (see http://bugzilla.mozilla.org/show_bug.cgi?id=41929#c61)

Kevin

Comment 5

[Kevin Teuscher]

As a side note, while I was investigating this I found that we allow a duplicate
server to get created with the same host and port using the account wizard
because of the following:
The account wizard checks to see if an account with the same username, host, and
type exist.  For news accounts, the first part of the e-mail address is used
When checking if the account exists, no matches are found because none of the
news servers have a username associated, so it passes the "account exists" testing.
When the account is actually created, news accounts automatically have the
username set to null, thus possibly matching an account that it didn't
previously in the "account exists" testing.
So I think what needs to be done is modify the "account exists" testing for news
servers to pass an empty username.

I'll file a different bug if you agree.

Kevin

Comment 6

[Kevin Teuscher]

(In reply to comment #5)
> As a side note, while I was investigating this I found that we allow a duplicate
> server to get created with the same host and port using the account wizard
> because of the following:
> The account wizard checks to see if an account with the same username, host, and
> type exist.  For news accounts, the first part of the e-mail address is used
> When checking if the account exists, no matches are found because none of the
> news servers have a username associated, so it passes the "account exists"
testing.
> When the account is actually created, news accounts automatically have the
> username set to null, thus possibly matching an account that it didn't
> previously in the "account exists" testing.
> So I think what needs to be done is modify the "account exists" testing for news
> servers to pass an empty username.
> 
> I'll file a different bug if you agree.
> 
> Kevin

This behavior predates the fixes in bug 41929 (I tested it in TB 0.7.1)

Comment 7

[Kevin Teuscher]

(In reply to comment #4)
> server like the system version of strcmp.  The crash comes because
> entry->username is empty (no usernames for news servers).
> In the old method that is still in the file (findServer), we use PL_strcmp. 
Actually, I'm not sure if it is entry->username or thisUsername, but the fix
works whichever it is.

Kevin

Comment 8

[David :Bienvenu]

Comment on attachment 158197 [details] [diff] [review]
wallpaper

Thx for looking at this. We're trying to get away from using these nsCRT
routines. It's better to explicitly check for null or else someone could come
along and get rid of the nsCRT...

Comment 9

[Kevin Teuscher]

(In reply to comment #8)
> (From update of attachment 158197 [details] [diff] [review])
> Thx for looking at this. We're trying to get away from using these nsCRT
> routines. It's better to explicitly check for null or else someone could come
> along and get rid of the nsCRT...
> 

So will a patch using PL_strcmp suffice or are you looking for a patch that
checks all of the items for null before sending to strcmp?

Comment 10

[David :Bienvenu]

PL_strcmp would be OK, as long as there's a big comment warning people not to
replace the PL_strcmp with strcmp because one or more of the args might be null.
Thx!

Comment 11

[Kevin Teuscher]

Attachment: use PL_strcmp

Changed to PL_strcmp
Changed the nsCRT::strcasecmp to PL_strcasecmp
Added note to warn about changing to strcmp

Comment 12

[David :Bienvenu]

fixed, thx, Kevin.