Closed
Bug 659871
Opened 13 years ago
Closed 13 years ago
nsStandardURL::SetPort() allows negative ports to be set
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
firefox7 | --- | fixed |
People
(Reporter: jesup, Assigned: jesup)
Details
(Whiteboard: [qa?])
Attachments
(2 files, 1 obsolete file)
769 bytes,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
1.35 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•13 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
Comment 2•13 years ago
|
||
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•13 years ago
|
||
Attachment #535274 -
Flags: review?(bzbarsky)
Comment 4•13 years ago
|
||
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•13 years ago
|
||
checked in as http://hg.mozilla.org/mozilla-central/rev/0cf4fa02c0f2
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment 6•13 years ago
|
||
Backed out due to perma-orange on XPCshell tests: http://tbpl.mozilla.org/?tree=Firefox&rev=0cf4fa02c0f2
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 9•13 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
Closed: 13 years ago → 13 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•13 years ago
|
status-firefox7:
--- → fixed
Comment 10•13 years ago
|
||
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•13 years ago
|
||
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 13•13 years ago
|
||
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•13 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•13 years ago
|
||
Adding qa+ for this to be re-evaluated for verification; if I misunderstood the reasoning please change back
Whiteboard: [qa-] → [qa+]
Comment 16•13 years ago
|
||
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•13 years ago
|
||
Assignee | ||
Updated•13 years ago
|
Attachment #554497 -
Attachment is obsolete: true
Assignee | ||
Comment 18•13 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 19•13 years ago
|
||
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.
Description
•