75973 - crash [@ nsMsgNewsFolder::GetNntpServer()] when trying to load bogus NNTP server URIs

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Message Display, defect, P1)

Description

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

Build ID: 4-13 (all OSs)

Summary: Crash in Account Wizard, with "secnews/netscape.communicator" as an 
NNTP server.

Steps to Reproduce:

1.  Spawn the Account Wizard, create an NNTP account.
2.  In the server window, enter "secnews/netscape.communicator" as your NNTP 
server.
3.  Click Finish.

Expected:

A non-existant news server is created in the Folder Pane, which is not 
subscribable)

Actual:

We crash.  Stack coming.

Note: The correct name for this server is "secnews.netscape.com".  In the above 
example, the user *might* be thinking they can put a newsgroup name in here, 
after the name of the server.  I highly doubt it, but in any case, we should 
either escape the "/". - string.replace("/", ""); Would that work?  I'm sure 
there's a better solution though.

Comment 1

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

Talkback is down, I'll step into a debug build and grab a stack.

Comment 2

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

Attachment: Stack, obviously, something in our JS code doesn't parse this character correctly.

Comment 3

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

Attachment: Better stack trace this time.

Comment 4

[Håkan Waara]

We should show an alert saying that we couldn't connect to the specified server.

Comment 5

[Håkan Waara]

I misunderstood, ignore my above comments. When I tried this I crashed 
immediately upon OKing the Account Wizard as noted.

I am investigating now...

Comment 6

[Håkan Waara]

*** Bug 75976 has been marked as a duplicate of this bug. ***

Comment 7

[Håkan Waara]

Attachment: Stephend's stacktraces to this bug are bogus. Attaching a good one.

Comment 8

[Håkan Waara]

So, this bug is really about bullet-proofing and making it so we don't load
corrupt server URIs. I will try my hand at it.

Comment 9

[Håkan Waara]

Fixing summary. Adding jpatel for possible talkback info.

Comment 10

[Håkan Waara]

I sent a patch/workaround and a description of it to Seth this past week. Still
waiting for info on how to proceed on this...

Comment 11

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

*** Bug 80091 has been marked as a duplicate of this bug. ***

Comment 12

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

taking back, working on the fix for this.

need to fix some news code and some account wizard stuff.

Comment 13

[Håkan Waara]

What!?  What about the patch I sent you two weeks ago that you still hasn't
commented?

Comment 14

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

calm down.  I just sent you private about your patch.

summary:  the first thing to do is to fix the account wizard.  working on that now.

Comment 15

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

for the account wizard problem, we are blocked bug 80519.

if that were fixed, we could land the attached patch.  

todo for this patch is to extend hostnameIsIllegal() to cover more than "/".

I think rfc 952 covers all the rules.  see 
http://www.netsys.com/sunmgr/1998-08/msg00178.html

of course, IP address would also be legal for server hostnames.

some form of hostname checking may already live in mozilla, so we should look 
for it first.

there still may be more crashers lurking after this patch.  for example, a 
bogus news://host/message-id or news://host or news://host/group url does not 
go through the account manager.  I'll have to try those cases out to see if 
more work needs to be done.

but in any case, I'm going to wait until #80519 is fixed.

Comment 16

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

Attachment: initial patch

Comment 17

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

moving to 0.9.2

this can wait.

Comment 18

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

new patch coming... (my old patch was the wild goose that was causing the 
invalid bugs that I logged.)

Comment 19

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

this patch can land (to prevent the crash) but I'll log a new bug on finishing 
hostnameIsIllegal()

racham, can you review?

Comment 20

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

Attachment: updated fix.

Comment 21

[Håkan Waara]

+  if (hostname == "") return true;
+
+  // make sure hostname is valid.
+  // XXX TODO do a complete check.  see rfc 952
+  if (hostname.indexOf("/") != -1) return true;

Maybe you could combine those checks, in a single if()?

+    if (!server) return NS_ERROR_FAILURE;

I think you should return NS_ERROR_NULL_POINTER if server turns out to be null.

Were there no callers you need to fix up to validate()?

r=hwaara

Comment 22

[racham]

Seth,

you may want to say 

if (!hostname) return true;

instead of 

if (hostname == "") return true;

r=racham.

Comment 23

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

thanks for the feedback, I'll create a new patch.

Comment 24

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

Attachment: new patch

Comment 25

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

here's a new patch, with more robust but not complete hostname checking.

it will prevent empty hostnames, or hostnames with any illegal characters.

but some illegal hostnames will still get through.  (see my comment in the 
patch.)

once this lands, I'll log a new bug to track that issue.

Comment 26

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

Before you commit, can you just change:

+ // bug hostname like "...." and "_" and ".111" will get by

into:

+ //but hostnames like "...." and "_" and ".111" will get by

Just for clarity, when/if others work on the code. (Sorry to nit-pick)

Comment 27

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

no no, thanks for catching my spelling / grammar mistakes.

I just wish I had you to proof read my bugzilla comments, news postings, email,
comments, code, etc.

there is a disconnect between my brain and my hands, I swear!

new patch on the way.

Comment 28

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

Attachment: patch

Comment 29

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

sr=bienvenu

#80855 logged for the remaining issue.

Comment 30

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

Verified FIXED on:
Mac OS 9.1 Build ID: 2001051508
RedHat 7.0 Build ID: 2001051508
Windows 2K Build ID: 2001051504

Entering an invalid news server name such as secnews/netscape.communicator
throws up an alert, "Please enter a valid hostname.", instead of crashing.