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

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mayhemer, Assigned: mayhemer)

Tracking

Trunk
Points:
---
Bug Flags:
blocking1.9.2 -
wanted1.9.2 +

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [sg:low spoof])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

8 years ago
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.
(Assignee)

Updated

8 years ago
blocking2.0: --- → ?
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Whiteboard: [sg:low spoof]

Updated

8 years ago
blocking2.0: ? → beta1
Flags: wanted1.9.2?
Flags: wanted1.9.2+
Flags: blocking1.9.2?
Flags: blocking1.9.2-

Updated

7 years ago
blocking2.0: beta1+ → beta2+
Needs to be fixed before final, don't think it needs to be in a beta, though.
blocking2.0: beta2+ → final+
(Assignee)

Comment 2

7 years ago
Created attachment 472477 [details] [diff] [review]
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
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.
(Assignee)

Comment 4

7 years ago
(In reply to comment #3)
> We could definitely use some better XPCOM parsing functions.
Do you know about any?
The point was we should create some.  We don't have them now.
(Assignee)

Comment 6

7 years ago
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.
(Assignee)

Comment 7

7 years ago
Created attachment 474044 [details] [diff] [review]
v1

- checking for only digits in the port string
- checking for positive value
Attachment #472477 - Attachment is obsolete: true
Attachment #474044 - Flags: review?(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
Attachment #474044 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

7 years ago
Created attachment 474362 [details] [diff] [review]
v1.1 [Check in comment 14]

Updated, thanks for review.
Attachment #474044 - Attachment is obsolete: true
Attachment #474362 - Flags: review+
(Assignee)

Comment 10

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

http://hg.mozilla.org/mozilla-central/rev/8f65173dacb0
Attachment #474362 - Attachment description: v1.1 → v1.1 [Check-in comment 10]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Depends on: 595525
(Assignee)

Updated

7 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Updated

7 years ago
Attachment #474362 - Attachment description: v1.1 [Check-in comment 10] → v1.1 [Back out comment 11]
(Assignee)

Comment 12

7 years ago
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.
(Assignee)

Comment 13

7 years ago
(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!
(Assignee)

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/491c6d05108f
Attachment #474362 - Attachment description: v1.1 [Back out comment 11] → v1.1 [Check in comment 14]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.