99081 - ExtractPortFrom sometimes finds ports that are not there

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Andreas Otte]

ExtractPortFrom sometimes finds ports that are not there. 

Parsing news:[e-mail] ends up with news as host and 3 as
port! That is clearly wrong. Test it with the given url and urltest as testprogram.

Comment 1

[Andreas Otte]

Attachment: patch to fix the problem

Comment 2

[benc]

being somewhat ignorant about the internals of what is going on, I'd like to ask:

Is this problem related to the fact that some URL schemes do not use "://"
("Common Internet Scheme Syntax")?

So this would be a problem with finger: and mailto:, and the fix would apply as
well?

Comment 3

[Andreas Otte]

Yes and No :-)

All these simple urltypes (like news, finger, about, ...) should use nsSimpleURI
or something similar implementing nsIURI. Usually there are no ports involved
and ExtractPortFrom is not used.

The other more complex urls use nsStdURL or something home grown as
implementation of nsIURL. 

The real bug here is visible in bug 91908, where we subject a simple uri (news)
to an nsIURL implementation. Really strange things can and do happen then. But
aside from the invalid use of nsStdURL in bug 91908 it demonstrated a bug in the
urlparser when going after ports. 

Fixing this bug *may* help fixing bug 91908 (so I marked it a blocker).

Comment 4

[Andreas Otte]

I'm a little bit confused now. According to RFCs I found news urls look like
simple urls (news.xxxxx) but if I look into the sources I see all kinds of
news:// type urls. There are however nntp urls in the RFCs which look like some
of the news urls seen in mozilla. If news urls in mozilla are all like news://
the fix in this bug should fix the assertions on attached news urls in mails.
Hopefully the result of the parsing process is still readable to mailnews ... 

Comment 5

[Phil Anderton]

Andreas is right - according to rfc1738, a news URL takes one of 2 forms,
news:<newsgroup-name> and news:<message-id>, whereas a nntp URL takes the form
nntp://<host>:<port>/<newsgroup-name>. As far as I can see, Moz does not support
the latter two forms, but incorrectly supports URLs of the form news:// which
appear to be invalid. Please change summary to reflect this.

Comment 6

[Phil Anderton]

Not a dependency, but also of interest is bug 49157.

Comment 7

[Andreas Otte]

That should be another bug.

Comment 8

[Doug Turner (:dougt)]

jeez.. I hate the name of this function.  Can you change at least the comment
above the function to state:

/* extracts first number from a string and assumes that this is the port number*/

or something similar?

with that, r=me

Comment 9

[Andreas Otte]

Attachment: updated comment per doug's request

Comment 10

[Darin Fisher]

it seems like you could eliminate the "error" flag by using an early "return -1;"
i think the resulting code would be a bit simpler.

fix that and sr=darin.

Comment 11

[Andreas Otte]

Attachment: incorporated darin's comments

Comment 12

[Andreas Otte]

fix checked in 

Comment 13

[Andreas Otte]

This patch caused a regression, see bug 99657 for details. That was fixed by
making the pop3 url canonial. However, that does not fix the real problem, we
need to make the new port detector sensible to ? as well. Moving the first
proposed patch from bug 99657 over to here.

Comment 14

[Andreas Otte]

Attachment: patch to fix the regression

Comment 15

[Doug Turner (:dougt)]

r=dougt?

Are there any other cases that we need to check for?  

Comment 16

[Andreas Otte]

Attachment: updated patch including # case

Comment 17

[Andreas Otte]

I haven't seen any url structured like this but there is a theoretical
possibility, so I also included the # case in the latest patch. That should do
it, that are the characters we are looking for in the urlparser (ParseAtPort
from nsStdURLParser.cpp). Thanks for the "heads up" Doug, can you r= again?

Comment 18

[Darin Fisher]

Comment on attachment 50885 [details] [diff] [review]
updated patch including # case

r/sr=darin

Comment 19

[Andreas Otte]

fix checked in

Comment 20

[benc]

Can someone give me pointers on urltest so I can learn it and verify this?

Andreas: as a side note, do we need new bugs about the fact that most internal
news URLs implement ("Common Internet Scheme Syntax") rather than being local
references to the default news server?

Comment 21

[Andreas Otte]

I described some of the workings of urltest in bug 95677. If you need more
please tell me.

In this case just do: 

urltest news:[e-mail] 

It will return something like:

news:[e-mail]
news,3B5C133C.2080505,,foobar.net,-1,/,,,,,,news://[e-mail]/ 

where the comma separated list gives the following:
scheme,username,password,host,port,directory,filename,fileextension,param,query,href,escaped
spec

If you want other checks, just verify with:

urltest http://www.mozilla.org:80?query
 or urltest http://www.mozilla.org:80#href


On the other stuff: No, I think there is already a bug about that, bug 49157 I
think.

Comment 22

[Boris Zbarsky [:bzbarsky]]

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

Comment 23

[Matthias Versen [:Matti]]

is this bug really fixed ? (-> bug 104156) 

Comment 24

[Andreas Otte]

This bug was about sometimes finding a port when there is no one. An example was
the  news url where I spotted it. That part is fixed. I didn't say we are able
to parse news urls correctly, we just don't do *this* error in parsing them any
more. See my comments about nsSimpleURL versus nsStdURL.

The dup is wrong I think, there is bug 89939 on the same issue as bug 104156,
but the information on bug 104156 is much better.