23096 - Specifying pop server with the port number appended crashes

Status: VERIFIED FIXED

(MailNews Core :: Networking, defect, P5)

Description

[Stuart Inglis]

When I specify a pop server or smtp server as localhost:port mozilla doesn't
accept it and later crashes.

Is there another way to specify the port number?

Comment 1

[Scott MacGregor]

stuart, there is a separate preference for setting the port on a server. Try
doing that. Look in your prefs.js and for the server in question add a pref that
looks like the following:
user_pref("mail.server.server1.port", 143);

where 143 is your port # and server1 is the name of the server in your
preferences (you should see lots of server prefs already there with that name).

It looks like the account manager needs to provide a slot for the port number so
users can enter the port in the UI. Re-assiging to alecf.

Comment 2

[Alec Flett]

I believe this is a dupe of something I already have, but I'm not sure.

Comment 3

[Alec Flett]

stuart, can you get a stack trace of the crash? I don't know why it would be
crashing, but I'd like to see where...

Comment 4

[Alec Flett]

well, since migration doesn't make this crash, and users aren't likely going to
do this, I'm holding off until m15.

Comment 5

[leger]

Adding "crash" keyword to all known open crasher bugs.

Comment 6

[Sebastian Späth]

I think average users would be confused to have an additional port slot with the 
POP host address (even if the default port is already pre-specified).
This would include the need to explain what they are for and that they usually 
shouldn't be touched.

It is better to have just one textfield in "host[:port]" format, so experienced 
users can choose different ports, while "grandma" wouldn't even notice that 
there's something complicated like ports.

Comment 7

[Alec Flett]

there's kind of an unwritten rule in software development: it's better to 
possibly confuse advanced users than to definately confuse novice users...  

imagine tech support on the phone to a user, "Ok, now where it has your 
hostname, yes isp.blah.com, put a colon, and then 394. no, I meant "after" as in 
in that same text field, not the next text field.. no don't put a space. yes a 
colon is the one that looks like a two periods, not a period and a comma, 
that's a semicolon... it doesn't work? read to me exactly character for 
character what the "hostname" field says..." etc etc...

not only that, but it's not at ALL obvious that putting :port at the end of a 
hostname means to use that port, whether your an advanced user or a novice 
user.... nowhere in the UI does it say ":port" or "put your port after the 
hostname with a colon" or anything wierd like that... and so I am against this 
idea.



the "port" setting will be in some kind of "advanced" dialog somewhere so gramma 
wouldn't even see it.

Comment 8

[David :Bienvenu]

David, could you try to reproduce this, figure out what's going on, and
hopefully fix it? thanks.

Comment 9

[davidmc]

Sure, I'll try to work this one out.  Sounds like fun.  But maybe someone can 
coach me on what is supposed to happen when I pick a random port number in order 
to reproduce the crashing?  I don't have any easy means to setup a server to 
actually use an arbitrary port number (never did it before).

However, the part about putting the port number somewhere in the prefs is easier, 
and I can have fun following the code path to see it actually gets used by the 
socket layer wherever that is.

Comment 10

[David :Bienvenu]

I think we crash before we attempt to make a connection to the bogus port.  
Thus, you don't need to configure any server. That's my guess, anyway, but 
actually trying it out should tell you.

Comment 11

[davidmc]

The following change in nsMsgIncomingServer.cpp seems to fix the problem; it 
replaces the macro which generated both GetHostName() and SetHostName(), with
explicit implementations which  qualify the hostname when getting.  If a colon is 
detected anywhere in the name, a zero is slammed over that byte and the result is 
written back to prefs.  Note this is clearly hacky, but less complex than going 
for some general scheme to qualify hostnames in a more general sense.

Maybe alecf can review this change and consider any complications.  My simple 
runtime test removes the bad behavior; but that's not very conclusive.

//NS_IMPL_SERVERPREF_STR(nsMsgIncomingServer, HostName, "hostname");
NS_IMETHODIMP
nsMsgIncomingServer::GetHostName(char **retval)
{
	nsresult rv = GetCharValue("hostname", retval);
	if ( NS_SUCCEEDED(rv) && retval && *retval)
	{
		char* name = *retval;
		int c = (unsigned char) *name;
		while ( c )
		{
			if ( c == ':' )
			{
				*name = 0; // clobber the first colon character
				SetCharValue("hostname", *retval); // write back to prefs
				break; // end while loop
			}
			c = *++name;
		}
	}
  return rv;
}
NS_IMETHODIMP
nsMsgIncomingServer::SetHostName(const char *chvalue)
{
  nsresult rv = SetCharValue("hostname", chvalue);
  return rv;
}

Comment 12

[davidmc]

Looks like another cast is needed for systems using signed chars:
c = (unsigned char) *++name;

Comment 13

[Alec Flett]

this looks great...
minor comments:
1) why use make c an integer and then cast? how about just making it a char,
thus making it match whatever signedness the platform is using (also allowing
the compiler to make extra optimizations since you're no longer changing the
storage class of the value in *name when you transfer it's value into c)
2) we should try interpreting the port number after the colon, and calling
SetPort() with it.
3) in SetHostName, I think similar logic should be used to what you wrote in
GetHostName - so that if I call SetHostName("hostname:40") that only "hostname"
actually gets written to the pref, (and SetPort() is also called)

Comment 14

[davidmc]

1) the 'int' is just long habit; char seems to do fine.
2) David Bienvenu agreed if a port number was present, it should be a default 
which gets overridden by a separate port number pref; so automatically calling 
SetPort() here would instead give precedence to this port number, right?
3) I was going to do something similar in SetHostName(), but then I noticed the 
argument was const, and I didn't want to cast away const or allocate another 
string (we do that way too much by the way).  I figured if getting side managed 
to put things to rights eventually, that might be enough.

Comment 15

[Alec Flett]

2) Yeah, that's true. I can go either way on this.
3) you are very right about the const char thing... but it does seem like at
this point, setting the hostname to "abc:40" _should_ override the port
preference, so we should just go ahead and set it. (otherwise, setting the port
to abc:40 would pretty much be exactly like setting it to abc)

Comment 16

[davidmc]

2) maybe handle the port number in 3) below instead
3) if the string contains a colon, then make a stack-based copy if small enough, 
or go to heap if too big; is there a limit on how long server names get?  I know 
there's probably a perfect string class for dividing the stack vs. heap aspect, 
but I don't pay any attention to the changing string architectures. Anything 
shoulds fine to me, and I can easily do it by hand in several lines of code.

Comment 17

[Alec Flett]

nsCAutoString is a stack-based class that goes to the heap with strings over 64
bytes, without you having to think about it. 
Usage is trivial
nsCAutoString myStr(someConstCharString);
myStr.Truncate(somePosition);

I don't believe there is a limit, or at least that we should impose one.

Comment 18

[davidmc]

Let's use nsCAutoString, then. Tomorrow I'll try a new version in my tree.

Comment 19

[David :Bienvenu]

these are mine now, I guess.

Comment 20

[David :Bienvenu]

Alec, do you have this patch in your tree to check in? If not, please assign 
back to me.

Comment 21

[Alec Flett]

I don't....

Comment 22

[David :Bienvenu]

moving to m17

Comment 23

[David :Bienvenu]

accepting

Comment 24

[selmer (gone)]

Mail Review recommends this bug as beta2 stopper.  Adding nsbeta2 keyword.

Comment 25

[Alec Flett]

I'm just going to fix this and get it over with :)

Comment 26

[Alec Flett]

reassigning to me, I have a fix.

Comment 27

[Alec Flett]

ok, fix is in.

Comment 28

[esther]

Alec, how do I test this.  
1.) Should I create a New account and give a bogus server name w/port like 
"nsmail:40" in the field for POP server name?   
2.) OR should I edit the prefs.js file and change the port to "40" (40 is just a 
random number)?

I did both of these and nothing bad happened, with the 1st one, the prefs.js 
file has the port string (which is a bogus port), but it got my mail anyway as 
did scenario 2.

If the port is bogus do we just ignore the bogus port and go to the default.
If the port is not bogus do we use the entered port number?  

Let me know.

Comment 29

[Alec Flett]

sorry for the delay. Talked to esther in the hallway:
- it should not crash
- if you put "hostname:9999" in your prefs as the hostname for a particular
server, you should be able to start mail and your prefs should now have
seperated the hostname and port out into "hostname" and 9999
- if you enter "hostname:9999" in the account wizard, your prefs should have
similar results.

Comment 30

[esther]

Verified.