Last Comment Bug 659871 - nsStandardURL::SetPort() allows negative ports to be set
: nsStandardURL::SetPort() allows negative ports to be set
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Randell Jesup [:jesup]
: Patrick McManus [:mcmanus]
Depends on:
  Show dependency treegraph
Reported: 2011-05-25 22:45 PDT by Randell Jesup [:jesup]
Modified: 2011-10-12 11:01 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

Simple patch (769 bytes, patch)
2011-05-25 23:52 PDT, Randell Jesup [:jesup]
bzbarsky: review+
Details | Diff | Splinter Review
Test for negative ports (1.12 KB, patch)
2011-08-19 12:02 PDT, Randell Jesup [:jesup]
no flags Details | Diff | Splinter Review
Add negative ports in URLs to tests (1.35 KB, patch)
2011-10-12 00:43 PDT, Randell Jesup [:jesup]
bzbarsky: review+
Details | Diff | Splinter Review

Description Randell Jesup [:jesup] 2011-05-25 22:45:07 PDT
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] 2011-05-25 23:01:06 PDT
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 [:bz] (still a bit busy) 2011-05-25 23:03:05 PDT
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] 2011-05-25 23:52:47 PDT
Created attachment 535274 [details] [diff] [review]
Simple patch
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2011-05-26 10:20:59 PDT
Comment on attachment 535274 [details] [diff] [review]
Simple patch

Please make that |port <= 0 && port != -1| and r=me.
Comment 5 Randell Jesup [:jesup] 2011-05-26 23:50:42 PDT
checked in as
Comment 6 Mounir Lamouri (:mounir) 2011-05-27 00:56:08 PDT
Backed out due to perma-orange on XPCshell tests:
Comment 7 Mounir Lamouri (:mounir) 2011-07-21 15:57:32 PDT
Randell, are you still working on this?
Comment 8 Randell Jesup [:jesup] 2011-07-21 18:30:36 PDT
Yes; just not high priority.
Comment 9 Randell Jesup [:jesup] 2011-07-21 18:42:42 PDT
Fix was checked in on 6/7/2011 when the other patch (which caused the orange) was checked in as patch
Comment 10 Virgil Dicu [:virgil] [QA] 2011-08-19 08:36:22 PDT
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] 2011-08-19 12:02:53 PDT
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).
Comment 12 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-22 13:55:37 PDT
qa+ for verification on Firefox 7 using comment 11.
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-26 12:11:23 PDT
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.

Comment 14 Randell Jesup [:jesup] 2011-09-26 20:51:50 PDT
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] 2011-09-26 20:54:49 PDT
Adding qa+ for this to be re-evaluated for verification; if I misunderstood the reasoning please change back
Comment 16 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-09-27 14:35:10 PDT
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] 2011-10-12 00:43:01 PDT
Created attachment 566458 [details] [diff] [review]
Add negative ports in URLs to tests
Comment 18 Randell Jesup [:jesup] 2011-10-12 00:48:28 PDT
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 [:bz] (still a bit busy) 2011-10-12 11:01:56 PDT
Comment on attachment 566458 [details] [diff] [review]
Add negative ports in URLs to tests


Note You need to log in before you can comment on or make changes to this bug.