Bug 1301621 - Parse URL ports as 16 bit; r=valentin
authorManish Goregaokar <manishsmail@gmail.com>
Fri, 09 Sep 2016 15:42:42 +0800
changeset 313911 483725b39bb2639b164badc7ce5298ba2574f9b2
parent 313910 a42e8a53dc39eee4d27c33b184f8f6d5dff367cb
child 313912 3b3e87b91108ced2173ae34357cfed8db7cfe6c0
push id32278
push usermanishearth@gmail.com
push dateWed, 14 Sep 2016 18:10:35 +0000
treeherderautoland@483725b39bb2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersvalentin
bugs1301621
milestone51.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1301621 - Parse URL ports as 16 bit; r=valentin MozReview-Commit-ID: 5FbRUsYzJdy
netwerk/base/nsStandardURL.cpp
netwerk/base/nsURLParsers.cpp
netwerk/test/unit/test_bug652761.js
netwerk/test/unit/test_invalidport.js
netwerk/test/unit/test_large_port.js
netwerk/test/unit/xpcshell.ini
testing/web-platform/meta/url/url-constructor.html.ini
testing/web-platform/meta/url/url-setters.html.ini
toolkit/components/search/tests/xpcshell/test_location_error.js
--- a/netwerk/base/nsStandardURL.cpp
+++ b/netwerk/base/nsStandardURL.cpp
@@ -1285,16 +1285,18 @@ nsStandardURL::GetHost(nsACString &resul
 {
     result = Host();
     return NS_OK;
 }
 
 NS_IMETHODIMP
 nsStandardURL::GetPort(int32_t *result)
 {
+    // should never be more than 16 bit
+    MOZ_ASSERT(mPort <= std::numeric_limits<uint16_t>::max());
     *result = mPort;
     return NS_OK;
 }
 
 // result may contain unescaped UTF-8 characters
 NS_IMETHODIMP
 nsStandardURL::GetPath(nsACString &result)
 {
@@ -1962,18 +1964,19 @@ nsStandardURL::SetPort(int32_t port)
 {
     ENSURE_MUTABLE();
 
     LOG(("nsStandardURL::SetPort [port=%d]\n", port));
 
     if ((port == mPort) || (mPort == -1 && port == mDefaultPort))
         return NS_OK;
 
-    // ports must be >= 0
-    if (port < -1) // -1 == use default
+    // ports must be >= 0 and 16 bit
+    // -1 == use default
+    if (port < -1 || port > std::numeric_limits<uint16_t>::max())
         return NS_ERROR_MALFORMED_URI;
 
     if (mURLType == URLTYPE_NO_AUTHORITY) {
         NS_WARNING("cannot set port on no-auth url");
         return NS_ERROR_UNEXPECTED;
     }
 
     InvalidateCache();
@@ -3118,17 +3121,18 @@ NS_IMETHODIMP
 nsStandardURL::Init(uint32_t urlType,
                     int32_t defaultPort,
                     const nsACString &spec,
                     const char *charset,
                     nsIURI *baseURI)
 {
     ENSURE_MUTABLE();
 
-    if (spec.Length() > (uint32_t) net_GetURLMaxLength()) {
+    if (spec.Length() > (uint32_t) net_GetURLMaxLength() ||
+        defaultPort > std::numeric_limits<uint16_t>::max()) {
         return NS_ERROR_MALFORMED_URI;
     }
 
     InvalidateCache();
 
     switch (urlType) {
     case URLTYPE_STANDARD:
         mParser = net_GetStdURLParser();
@@ -3169,16 +3173,21 @@ nsStandardURL::Init(uint32_t urlType,
 
 NS_IMETHODIMP
 nsStandardURL::SetDefaultPort(int32_t aNewDefaultPort)
 {
     ENSURE_MUTABLE();
 
     InvalidateCache();
 
+    // should never be more than 16 bit
+    if (aNewDefaultPort >= std::numeric_limits<uint16_t>::max()) {
+        return NS_ERROR_MALFORMED_URI;
+    }
+
     // If we're already using the new default-port as a custom port, then clear
     // it off of our mSpec & set mPort to -1, to indicate that we'll be using
     // the default from now on (which happens to match what we already had).
     if (mPort == aNewDefaultPort) {
         ReplacePortInSpec(-1);
         mPort = -1;
     }
     mDefaultPort = aNewDefaultPort;
--- a/netwerk/base/nsURLParsers.cpp
+++ b/netwerk/base/nsURLParsers.cpp
@@ -601,17 +601,17 @@ nsAuthURLParser::ParseServerInfo(const c
             }
             else {
                 const char* nondigit = NS_strspnp("0123456789", buf.get());
                 if (nondigit && *nondigit)
                     return NS_ERROR_MALFORMED_URI;
 
                 nsresult err;
                 *port = buf.ToInteger(&err);
-                if (NS_FAILED(err) || *port < 0)
+                if (NS_FAILED(err) || *port < 0 || *port > std::numeric_limits<uint16_t>::max())
                     return NS_ERROR_MALFORMED_URI;
             }
         }
     }
     else {
         // serverinfo = <hostname>
         SET_RESULT(hostname, 0, serverinfoLen);
         if (port)
--- a/netwerk/test/unit/test_bug652761.js
+++ b/netwerk/test/unit/test_bug652761.js
@@ -1,20 +1,17 @@
 // This is just a crashtest for a url that is rejected at parse time (port 80,000)
 
 Cu.import("resource://gre/modules/NetUtil.jsm");
 
-function completeTest(request, data, ctx)
+function run_test()
 {
+    // Bug 1301621 makes invalid ports throw
+    Assert.throws(() => {
+        var chan = NetUtil.newChannel({
+          uri: "http://localhost:80000/",
+          loadUsingSystemPrincipal: true
+        });
+    }, "invalid port");
+
     do_test_finished();
 }
 
-function run_test()
-{
-    var chan = NetUtil.newChannel({
-      uri: "http://localhost:80000/",
-      loadUsingSystemPrincipal: true
-    });
-    var httpChan = chan.QueryInterface(Components.interfaces.nsIHttpChannel);
-    httpChan.asyncOpen2(new ChannelListener(completeTest,httpChan, CL_EXPECT_FAILURE));
-    do_test_pending();
-}
-
deleted file mode 100644
--- a/netwerk/test/unit/test_invalidport.js
+++ /dev/null
@@ -1,38 +0,0 @@
-// This is essentially a crashtest for accessing an out of range port
-// Perform the async open several times in order to induce exponential
-// scheduling behavior bugs.
-
-Cu.import("resource://gre/modules/NetUtil.jsm");
-
-var CC = Components.Constructor;
-
-var counter = 0;
-const iterations = 10;
-
-var listener = {
-  onStartRequest: function test_onStartR(request, ctx) {
-  },
-
-  onDataAvailable: function test_ODA() {
-    do_throw("Should not get any data!");
-  },
-
-  onStopRequest: function test_onStopR(request, ctx, status) {
-    if (counter++ == iterations)
-      do_test_finished();
-    else
-      execute_test();
-  },
-};
-
-function run_test() {
-  execute_test();
-  do_test_pending();
-}
-
-function execute_test() {
-  var chan = NetUtil.newChannel({uri: "http://localhost:75000", loadUsingSystemPrincipal: true});
-  chan.QueryInterface(Ci.nsIHttpChannel);
-  chan.asyncOpen2(listener);
-}
-
new file mode 100644
--- /dev/null
+++ b/netwerk/test/unit/test_large_port.js
@@ -0,0 +1,36 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// Ensure that non-16-bit URIs are rejected
+
+"use strict";
+
+var Cc = Components.classes;
+var Ci = Components.interfaces;
+
+const StandardURL = Components.Constructor("@mozilla.org/network/standard-url;1",
+                                           "nsIStandardURL",
+                                           "init");
+function run_test()
+{
+    // Bug 1301621 makes invalid ports throw
+    Assert.throws(() => {
+        new StandardURL(Ci.nsIStandardURL.URLTYPE_AUTHORITY, 65536,
+                "http://localhost", "UTF-8", null)
+    }, "invalid port during creation");
+    let url = new StandardURL(Ci.nsIStandardURL.URLTYPE_AUTHORITY, 65535,
+                              "http://localhost", "UTF-8", null)
+                .QueryInterface(Ci.nsIStandardURL)
+
+    Assert.throws(() => {
+        url.setDefaultPort(65536);
+    }, "invalid port in setDefaultPort");
+    Assert.throws(() => {
+        url.port = 65536;
+    }, "invalid port in port setter");
+
+    do_check_eq(url.QueryInterface(Ci.nsIURI).port, -1);
+    do_test_finished();
+}
+
--- a/netwerk/test/unit/xpcshell.ini
+++ b/netwerk/test/unit/xpcshell.ini
@@ -227,18 +227,18 @@ skip-if = bits != 32
 [test_idn_blacklist.js]
 [test_idn_urls.js]
 [test_idna2008.js]
 # IDNA2008 depends on ICU, not available on android
 skip-if = os == "android"
 [test_immutable.js]
 skip-if = !hasNode
 run-sequentially = node server exceptions dont replay well
-[test_invalidport.js]
 [test_localstreams.js]
+[test_large_port.js]
 [test_mismatch_last-modified.js]
 [test_MIME_params.js]
 [test_mozTXTToHTMLConv.js]
 [test_multipart_byteranges.js]
 [test_multipart_streamconv.js]
 [test_multipart_streamconv_missing_lead_boundary.js]
 [test_nestedabout_serialize.js]
 [test_net_addr.js]
--- a/testing/web-platform/meta/url/url-constructor.html.ini
+++ b/testing/web-platform/meta/url/url-constructor.html.ini
@@ -232,19 +232,16 @@
     expected: FAIL
 
   [Parsing: <sc://ñ.test/> against <about:blank>]
     expected: FAIL
 
   [Parsing: <file:..> against <http://www.example.com/test>]
     expected: FAIL
 
-  [Parsing: <http://f:999999/c> against <http://example.org/foo/bar>]
-    expected: FAIL
-
   [Parsing: <http://www/foo%2Ehtml> against <about:blank>]
     expected: FAIL
 
   [Parsing: <http://example.com/foo/%2e%2> against <about:blank>]
     expected: FAIL
 
   [Parsing: <http://example.com/foo/%2e./%2e%2e/.%2e/%2e.bar> against <about:blank>]
     expected: FAIL
--- a/testing/web-platform/meta/url/url-setters.html.ini
+++ b/testing/web-platform/meta/url/url-setters.html.ini
@@ -70,19 +70,16 @@
     expected: FAIL
 
   [Setting <view-source+http://example.net/path>.host = 'example.com\\stuff' \\ is not a delimiter for non-special schemes, and it’s invalid in a domain]
     expected: FAIL
 
   [Setting <view-source+http://example.net/path>.host = 'example.com:8080stuff2' Anything other than ASCII digit stops the port parser in a setter but is not an error]
     expected: FAIL
 
-  [Setting <http://example.net/path>.host = 'example.com:65536' Port numbers are 16 bit integers, overflowing is an error. Hostname is still set, though.]
-    expected: FAIL
-
   [Setting <view-source+http://example.net/foo>.hostname = '' The empty host is OK for non-special schemes]
     expected: FAIL
 
   [Setting <a:/foo>.hostname = 'example.net' Path-only URLs can gain a host]
     expected: FAIL
 
   [Setting <http://example.net:8080>.hostname = '0x7F000001' IPv4 address syntax is normalized]
     expected: FAIL
@@ -100,19 +97,16 @@
     expected: FAIL
 
   [Setting <http://example.net:8080>.port = '' Port number is unchanged if empty in the new value. Note: this may change, see https://github.com/whatwg/url/pull/113]
     expected: FAIL
 
   [Setting <view-source+http://example.net/path>.port = '8080stuff2' Anything other than ASCII digit stops the port parser in a setter but is not an error]
     expected: FAIL
 
-  [Setting <http://example.net:8080/path>.port = '65536' Port numbers are 16 bit integers, overflowing is an error]
-    expected: FAIL
-
   [Setting <unix:/run/foo.socket?timeout=10>.pathname = '/var/log/../run/bar.socket']
     expected: FAIL
 
   [Setting <http://example.net/home?lang=fr#nav>.pathname = '\\a\\%2E\\b\\%2e.\\c' \\ is a segment delimiter for 'special' URLs]
     expected: FAIL
 
   [Setting <view-source+http://example.net/home?lang=fr#nav>.pathname = '\\a\\%2E\\b\\%2e.\\c' \\ is *not* a segment delimiter for non-'special' URLs]
     expected: FAIL
--- a/toolkit/components/search/tests/xpcshell/test_location_error.js
+++ b/toolkit/components/search/tests/xpcshell/test_location_error.js
@@ -1,16 +1,16 @@
 /* Any copyright is dedicated to the Public Domain.
    http://creativecommons.org/publicdomain/zero/1.0/ */
 
 function run_test() {
   installTestEngine();
 
-  // using a port > 2^32 causes an error to be reported.
-  let url = "http://localhost:111111111";
+  // We use an invalid port that parses but won't open
+  let url = "http://localhost:0";
 
   Services.prefs.setCharPref("browser.search.geoip.url", url);
   Services.search.init(() => {
     try {
       Services.prefs.getCharPref("browser.search.countryCode");
       ok(false, "not expecting countryCode to be set");
     } catch (ex) {}
     // should have an error recorded.