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

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

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...

Comment 2

[Boris Zbarsky [:bzbarsky]]

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.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Simple patch

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 535274 [details] [diff] [review]
Simple patch

Please make that |port <= 0 && port != -1| and r=me.

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

checked in as http://hg.mozilla.org/mozilla-central/rev/0cf4fa02c0f2

Comment 6

[Mounir Lamouri (:mounir)]

Backed out due to perma-orange on XPCshell tests:
http://tbpl.mozilla.org/?tree=Firefox&rev=0cf4fa02c0f2

Comment 7

[Mounir Lamouri (:mounir)]

Randell, are you still working on this?

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

Yes; just not high priority.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 10

[Virgil Dicu [:virgil] [QA]]

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?

Comment 11

[Randell Jesup [:jesup] (needinfo me)]

Attachment: 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).

Comment 12

[u279076]

qa+ for verification on Firefox 7 using comment 11.

Comment 13

[u279076]

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

Comment 14

[Randell Jesup [:jesup] (needinfo me)]

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

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

Adding qa+ for this to be re-evaluated for verification; if I misunderstood the reasoning please change back

Comment 16

[u279076]

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.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Add negative ports in URLs to tests

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

r=me

Comment 20

[Pulsebot]

Backout by ffxbld:
https://hg.mozilla.org/releases/mozilla-release/rev/9077903ebbb8
Backed out, changeset 0cf4fa02c0f2, because of orange.