537381 - -1 as port number cause default port to be used, should fail to load

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Honza Bambas (:mayhemer)]

This is a leftover from bug 479485 to be fixed.

We should add an is-positive (or even better is > 0) check for the port number.

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Needs to be fixed before final, don't think it needs to be in a beta, though.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: wip 1

This is a work in progress patch.  I updated the test and went further and find out, that URL "http://foo.com:1234:80/bar" is correctly parsed and internally represented as "http://foo.com:1234/bar" - the ":80" from the port portion of the URL is read but then ignored.  nsCAutoString::ToInteger is responsible for that.

This is another example where we are completely failing to safely parse a string.  I would like to use something better for parsing here, like lexical analyzer. See [1].

[1] http://groups.google.ca/group/mozilla.dev.platform/browse_thread/thread/47bf5b4cb8203053

Comment 3

[Boris Zbarsky [:bzbarsky]]

We could definitely use some better XPCOM parsing functions.

Note that the DOM no longer uses the ns(C)String::ToInteger gunk, since it just didn't do what the DOM needed.

Comment 4

[Honza Bambas (:mayhemer)]

(In reply to comment #3)
> We could definitely use some better XPCOM parsing functions.
Do you know about any?

Comment 5

[Boris Zbarsky [:bzbarsky]]

The point was we should create some.  We don't have them now.

Comment 6

[Honza Bambas (:mayhemer)]

I'll simply update the patch to check the string is whole just numbers and then use ToInteger on it.  Simple and quick, but also very dirty.

Comment 7

[Honza Bambas (:mayhemer)]

Attachment: v1

- checking for only digits in the port string
- checking for positive value

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 474044 [details] [diff] [review]
v1

>+++ b/netwerk/base/src/nsURLParsers.cpp
>+                const char* nondigit = NS_strspnp("0123456789", buf.BeginReading());

.get() please, since you actually want a null-terminated string.

r=me with that

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [Check in comment 14]

Updated, thanks for review.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 474362 [details] [diff] [review]
v1.1 [Check in comment 14]

http://hg.mozilla.org/mozilla-central/rev/8f65173dacb0

Comment 11

[Phil Ringnalda (:philor)]

Backed out for:

http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284217060.1284218020.18295.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284216191.1284217040.14302.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215868.1284216788.13151.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215765.1284216810.13218.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215991.1284216655.12771.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215991.1284216924.13798.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215605.1284217545.16230.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215491.1284216422.11484.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215490.1284216321.11209.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215491.1284216157.10391.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215491.1284216444.11697.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1284215521.1284217737.16918.gz

Comment 12

[Honza Bambas (:mayhemer)]

So, it looks like fixing bug 595525 will take some time, so I'll disable the failing test for now.  The test is nothing critical.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to comment #12)
> So, it looks like fixing bug 595525 will take some time, so I'll disable the
> failing test for now.  The test is nothing critical.

Taking back... it is not my test that is failing!

Comment 14

[Honza Bambas (:mayhemer)]

Comment on attachment 474362 [details] [diff] [review]
v1.1 [Check in comment 14]

http://hg.mozilla.org/mozilla-central/rev/491c6d05108f