nsStandardURL::SetPort() allows negative ports to be set

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jesup, Assigned: jesup)

Tracking

Trunk
x86
All
Points:
---

Firefox Tracking Flags

(firefox7 fixed)

Details

(Whiteboard: [qa?])

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Follow-on noticed in working bug 125608.

nsStandardURL::SetPort() allows you to set negative values for mPort other than -1 (which is 'unset').  This would result in an invalid URL.
(Assignee)

Comment 1

6 years ago
Should I add this:
        NS_ABORT_IF_FALSE(mPort > 0 || mPort == -1, "Bogus mPort");

Or return an NS_ERROR?  It appears most callers don't check the return value...
Status: NEW → ASSIGNED
Just return an error if a bogus port value is passed in.  You can't assert anything about what people will pass to a public API like this.
(Assignee)

Comment 3

6 years ago
Created attachment 535274 [details] [diff] [review]
Simple patch
Attachment #535274 - Flags: review?(bzbarsky)
Comment on attachment 535274 [details] [diff] [review]
Simple patch

Please make that |port <= 0 && port != -1| and r=me.
Attachment #535274 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 5

6 years ago
checked in as http://hg.mozilla.org/mozilla-central/rev/0cf4fa02c0f2
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Backed out due to perma-orange on XPCshell tests:
http://tbpl.mozilla.org/?tree=Firefox&rev=0cf4fa02c0f2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Randell, are you still working on this?
Assignee: nobody → rjesup
(Assignee)

Comment 8

6 years ago
Yes; just not high priority.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 9

6 years ago
Fix was checked in on 6/7/2011 when the other patch (which caused the orange) was checked in as patch http://hg.mozilla.org/mozilla-central/rev/a44485cd1682
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
(Assignee)

Updated

6 years ago
status-firefox7: --- → fixed
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0

Could you provide a clear set of STRs or a test case in order to verify this issue in QA?
(Assignee)

Comment 11

6 years ago
Created attachment 554497 [details] [diff] [review]
Test for negative ports

You'd need to write a short program to create a URL and then do SetPort(-10) on it.  I've attached a patch to test_URIs.js that tests the behavior - it passes on current trunk.  It should fail without this fix.  Note that test_URIs.js changed a fair bit, so you'll probably have to merge it by hand only an older one (should be easy).
qa+ for verification on Firefox 7 using comment 11.
Whiteboard: [qa+]
Based on comment 11, I don't think QA can easily verify this bug fix. If someone has the time to code and attach the "short program" required to verify this fix, please do so.

Thanks
Whiteboard: [qa+] → [qa-]
(Assignee)

Comment 14

6 years ago
Anthony - I think just applying that testcode patch onto an older test_URIs.js would let you test it ("cd checkoutdir; patch -p1 </tmp/attachment_554497"), then run the xpcshell tests - This should do it:

$ cd objdir
$ make SOLO_FILE="test_URIs.js" -C netwerk/test/ check-one
(Assignee)

Comment 15

6 years ago
Adding qa+ for this to be re-evaluated for verification; if I misunderstood the reasoning please change back
Whiteboard: [qa-] → [qa+]
Resetting this to qa? so we can evaluate if this is doable by QA. I suspect not, given the demands of the rapid release schedule. I would appreciate if someone who is already set up to test as per comment 15 could verify this fix.
Whiteboard: [qa+] → [qa?]
(Assignee)

Comment 17

6 years ago
Created attachment 566458 [details] [diff] [review]
Add negative ports in URLs to tests
(Assignee)

Updated

6 years ago
Attachment #554497 - Attachment is obsolete: true
(Assignee)

Comment 18

6 years ago
Comment on attachment 566458 [details] [diff] [review]
Add negative ports in URLs to tests

My only issue with adding this is: do we really need test setting the port for all those (absolute) http URLs?  But it's likely down in the noise.  Also, probably a new bug should be opened to properly track this test as it's checked in.
Attachment #566458 - Flags: review?(bzbarsky)
Comment on attachment 566458 [details] [diff] [review]
Add negative ports in URLs to tests

r=me
Attachment #566458 - Flags: review?(bzbarsky) → review+
You need to log in before you can comment on or make changes to this bug.