Issue metadata
Sign in to add a comment
|
KURL does not allow using port 65535 (it considers it invalid). |
||||||||||||||||||||||
Issue descriptionSee also the related bug https://bugs.chromium.org/p/chromium/issues/detail?id=605367#c4. 65535 is a valid private port. The current code also looks sketchy in that it considers any port > 65535 as 6553, under the assumption that 65535 is an invalid port number [1] // We treat URL's with out-of-range port numbers as invalid URLs, and they will // be rejected by the canonicalizer. KURL.cpp will allow them in parsing, but // return invalidPortNumber from this port() function, so we mirror that behavior here. unsigned short KURL::port() const { if (!m_isValid || m_parsed.port.len <= 0) return 0; ASSERT(!m_string.isNull()); int port = m_string.is8Bit() ? url::ParsePort(asURLChar8Subtle(m_string), m_parsed.port) : url::ParsePort(m_string.characters16(), m_parsed.port); ASSERT(port != url::PORT_UNSPECIFIED); // Checked port.len <= 0 before. if (port == url::PORT_INVALID || port > maximumValidPortNumber) // Mimic KURL::port() port = invalidPortNumber; return static_cast<unsigned short>(port); } [1] https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit/Source/platform/weborigin/KURL.cpp&q=platform/weborigin/KURL.cpp&sq=package:chromium&type=cs&l=394
,
Jun 15 2016
,
Jun 16 2016
FTR, https://chromium.googlesource.com/chromium/src/+/b498e0d4bb8ef9fc324e33a1020c520ed9ee6f82%5E%21/ is the patch which introduced the special value.
,
Jun 19 2017
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. If you change it back, also remove the "Hotlist-Recharge-Cold" label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 20 2017
,
Jun 20 2018
This issue has been Available for over a year. If it's no longer important or seems unlikely to be fixed, please consider closing it out. If it is important, please re-triage the issue. Sorry for the inconvenience if the bug really should have been left as Available. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jun 21 2018
Wow. That's... wow. Confirmed that http://127.0.0.1:65535/ works in Firefox and gives ERR_UNSAFE_PORT in Chrome. Same with WebSockets (except they are blocked early inside Blink with a call to net::IsPortAllowedForScheme). I'm tempted to fix the parsing part but not remove 65535 from the invalid port list (since that is shared with //net, and we don't know what the side-effects would be). While I was poking at this, I noticed that url::ParsePort contains the most convoluted implementation of StringToInt ever. Is it okay to delegate that stuff to base::StringToInt?
,
Jun 21 2018
Thanks taking this Adam! On the topic of base::StringToInt(), that should not be used in network code. For details see: https://chromium.googlesource.com/chromium/src/+/cb8b61b491aad9c3b36dbfd705d52a7bfc3c2dda/net/base/parse_number.h#20
,
Jun 22 2018
#8 I didn't spot those issues with StringToInt. Thanks for the warning. Since net::ParseInt32 doesn't have a StringPiece16 variant, I think I will leave url::ParsePort alone. It is working, after all.
,
Jun 26 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a9aee5fa4f278100aeafc4821c4247e0b9002806 commit a9aee5fa4f278100aeafc4821c4247e0b9002806 Author: Adam Rice <ricea@chromium.org> Date: Tue Jun 26 04:24:56 2018 Add CHECKs to KURL::Port() Add CHECK statements to KURL::Port() to verify that it is behaving as expected, in preparation for removing unnecessary code. The CHECKs will be replaced with DCHECKs in a followup CL after there's been time for any crashes to appear. BUG= 606462 Change-Id: Ib3bcc738005de27201435f2c62aa4056965d5c91 Reviewed-on: https://chromium-review.googlesource.com/1111879 Reviewed-by: Eric Roman <eroman@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Commit-Queue: Adam Rice <ricea@chromium.org> Cr-Commit-Position: refs/heads/master@{#570330} [modify] https://crrev.com/a9aee5fa4f278100aeafc4821c4247e0b9002806/third_party/blink/renderer/platform/weborigin/kurl.cc
,
Jun 27 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/70453d7b57f824014b748fd7e556eb5a767a324d commit 70453d7b57f824014b748fd7e556eb5a767a324d Author: Adam Rice <ricea@chromium.org> Date: Wed Jun 27 01:42:58 2018 Add unit tests for KURL::Port() Test the various ways of setting or changing the port on a blink::KURL and verify that KURL::Port() returns the correct answers in each case. The results are different depending on how the port was set: the constructors are strict about disallowing invalid ports. SetPort() converts any input to a valid port. SetHostAndPort() is somewhere inbetween. Bug: 606462 Change-Id: I1533d47aa113d2bb4482cdfe17e682a5da56649a Reviewed-on: https://chromium-review.googlesource.com/1114572 Reviewed-by: Tom Sepez <tsepez@chromium.org> Commit-Queue: Adam Rice <ricea@chromium.org> Cr-Commit-Position: refs/heads/master@{#570620} [modify] https://crrev.com/70453d7b57f824014b748fd7e556eb5a767a324d/third_party/blink/renderer/platform/weborigin/kurl_test.cc
,
Jun 28 2018
No crashes so far. Probably safe to leave the CHECKs in for another week, as long as I remove them before the branch.
,
Jul 5
The NextAction date has arrived: 2018-07-05
,
Jul 5
Still no crashes. I'm cleaning it up.
,
Jul 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/41d010e8a4d7622bc4e85f3629ac6a297119c2f3 commit 41d010e8a4d7622bc4e85f3629ac6a297119c2f3 Author: Adam Rice <ricea@chromium.org> Date: Wed Jul 18 04:44:52 2018 KURL::Port(): Remove unneeded constants Remove the constants kMaximumValidPortNumber and kInvalidPortNumber from kurl.cc as they are confusing and don't actually do anything. Also change remaining CHECKs in KURL::Port back to DCHECKs now that we have confidence the change is safe. Also make the method comment less confusing. BUG= 606462 Change-Id: I4719b8611cc6f3f0c5f488d8c86f3b6b8ad6d88f Reviewed-on: https://chromium-review.googlesource.com/1127083 Commit-Queue: Adam Rice <ricea@chromium.org> Reviewed-by: Tom Sepez <tsepez@chromium.org> Reviewed-by: Kinuko Yasuda <kinuko@chromium.org> Reviewed-by: Eric Roman <eroman@chromium.org> Cr-Commit-Position: refs/heads/master@{#575942} [modify] https://crrev.com/41d010e8a4d7622bc4e85f3629ac6a297119c2f3/third_party/blink/renderer/platform/weborigin/kurl.cc [modify] https://crrev.com/41d010e8a4d7622bc4e85f3629ac6a297119c2f3/third_party/blink/renderer/platform/weborigin/kurl.h
,
Jul 19
,
Jul 19
Awesome! Thanks, Adam!
,
Jul 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/9710afc35096fd4554bdd646a3aa9eb612f0e38f commit 9710afc35096fd4554bdd646a3aa9eb612f0e38f Author: Matt Menke <mmenke@chromium.org> Date: Fri Jul 20 15:23:42 2018 Allow connections to port 65535. KURL used to use this to mean invalid port, but that behavior has been removed. BUG= 606462 Change-Id: I320aace82e3065c952ba3ae7d88114460dc44a05 Reviewed-on: https://chromium-review.googlesource.com/1143685 Reviewed-by: Bill Budge <bbudge@chromium.org> Reviewed-by: Ryan Hamilton <rch@chromium.org> Reviewed-by: Adam Rice <ricea@chromium.org> Commit-Queue: Matt Menke <mmenke@chromium.org> Cr-Commit-Position: refs/heads/master@{#576868} [modify] https://crrev.com/9710afc35096fd4554bdd646a3aa9eb612f0e38f/net/base/port_util.cc [modify] https://crrev.com/9710afc35096fd4554bdd646a3aa9eb612f0e38f/ppapi/tests/test_websocket.cc |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by mmenke@chromium.org
, Jun 13 2016