Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(406)

Issue 1177933002: Resolve RFC 6761 localhost names to loopback (Closed)

Created:
5 years, 6 months ago by estark
Modified:
5 years, 6 months ago
CC:
chromium-reviews, cbentzel+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Resolve RFC 6761 localhost names to loopback Per section 6.3, HostResolverImpl should recognize localhost names as special and resolve as loopback without sending the query over the network. This CL also modifies IsLocalhost() to allow trailing dots (bug 460331), so that IsLocalhost() can be used to detect localhost names that should resolve to loopback. BUG=496479, 460331 Committed: https://crrev.com/d1bc206e4635403d342e40d6e04845c029f1ada7 Cr-Commit-Position: refs/heads/master@{#335402}

Patch Set 1 #

Total comments: 8

Patch Set 2 : add unit tests for IsLocalhost change #

Patch Set 3 : rsleevi, mmenke comments #

Total comments: 7

Patch Set 4 : mmenke comments #

Total comments: 1

Patch Set 5 : reshuffle tests #

Patch Set 6 : mmenke's suggestion: ResolveLocalHostname(name, &addresses) #

Patch Set 7 : style tweaks, simplification #

Total comments: 10

Patch Set 8 : rsleevi comments #

Total comments: 2

Patch Set 9 : rebase #

Patch Set 10 : namespace qualification for EndsWith #

Patch Set 11 : resolve test server addresses to IPv4 #

Total comments: 2

Patch Set 12 : rsleevi nit #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -48 lines) Patch
M net/base/net_util.h View 1 2 3 4 5 6 7 8 2 chunks +13 lines, -2 lines 0 comments Download
M net/base/net_util.cc View 1 2 3 4 5 6 7 8 9 4 chunks +55 lines, -18 lines 0 comments Download
M net/base/net_util_unittest.cc View 1 2 3 4 5 6 7 6 chunks +113 lines, -0 lines 0 comments Download
M net/dns/host_resolver_impl.h View 1 2 2 chunks +8 lines, -1 line 0 comments Download
M net/dns/host_resolver_impl.cc View 1 2 3 4 5 6 7 8 5 chunks +35 lines, -16 lines 0 comments Download
M net/dns/host_resolver_impl_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +45 lines, -11 lines 0 comments Download
M net/test/spawned_test_server/base_test_server.cc View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +7 lines, -0 lines 0 comments Download

Messages

Total messages: 44 (11 generated)
estark
ttuttle, could you please take a look?
5 years, 6 months ago (2015-06-11 00:24:19 UTC) #2
estark
added a few unit tests for net::IsLocalhost
5 years, 6 months ago (2015-06-11 00:28:59 UTC) #3
Ryan Sleevi
https://codereview.chromium.org/1177933002/diff/1/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/1/net/base/net_util.cc#newcode737 net/base/net_util.cc:737: host == "localhost6." || host == "localhost6.localdomain6." || :( ...
5 years, 6 months ago (2015-06-11 00:34:03 UTC) #5
mmenke
https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/1/net/dns/host_resolver_impl.cc#newcode2223 net/dns/host_resolver_impl.cc:2223: hostname = kLocalhost; Peppering this file with multiple localhost ...
5 years, 6 months ago (2015-06-11 00:36:51 UTC) #7
estark
Thanks sleevi, mmenke. See question inline about what to do with ADDRESS_FAMILY_UNSPECIFIED. https://codereview.chromium.org/1177933002/diff/1/net/base/net_util.cc File net/base/net_util.cc ...
5 years, 6 months ago (2015-06-11 02:35:32 UTC) #8
mmenke
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc#newcode2174 net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due ...
5 years, 6 months ago (2015-06-11 14:45:59 UTC) #9
estark
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc#newcode2174 net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due ...
5 years, 6 months ago (2015-06-11 17:16:48 UTC) #10
mmenke
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc#newcode2174 net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due ...
5 years, 6 months ago (2015-06-11 17:32:25 UTC) #11
estark
https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/1177933002/diff/40001/net/dns/host_resolver_impl.cc#newcode2174 net/dns/host_resolver_impl.cc:2174: // If the family was restricted to IPv4 due ...
5 years, 6 months ago (2015-06-11 22:25:26 UTC) #13
Ryan Sleevi
https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#newcode151 net/base/net_util.cc:151: std::string lowercased_host = base::StringToLowerASCII(host); Suggestion: To reduce comparisons for ...
5 years, 6 months ago (2015-06-12 01:00:04 UTC) #14
estark
https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/140001/net/base/net_util.cc#newcode151 net/base/net_util.cc:151: std::string lowercased_host = base::StringToLowerASCII(host); On 2015/06/12 01:00:04, Ryan Sleevi ...
5 years, 6 months ago (2015-06-12 05:59:46 UTC) #15
Ryan Sleevi
I think this LGTM. Make sure to ping ttuttle (surprised he hasn't replied), perhaps on ...
5 years, 6 months ago (2015-06-12 23:10:04 UTC) #16
Deprecated (see juliatuttle)
I'll take a look on Monday; I was holding off while Matt's comments were dealt ...
5 years, 6 months ago (2015-06-13 00:39:09 UTC) #17
estark
On 2015/06/13 00:39:09, ttuttle wrote: > I'll take a look on Monday; I was holding ...
5 years, 6 months ago (2015-06-16 02:15:18 UTC) #18
Deprecated (see juliatuttle)
lgtm modulo one question. https://codereview.chromium.org/1177933002/diff/160001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/1177933002/diff/160001/net/base/net_util.cc#newcode157 net/base/net_util.cc:157: bool IsNormalizedLocalhostTLD(const std::string& host) { ...
5 years, 6 months ago (2015-06-16 14:53:40 UTC) #19
estark
ttuttle: thank you mmenke: do you want to take another look before I land this? ...
5 years, 6 months ago (2015-06-16 15:40:15 UTC) #20
mmenke
On 2015/06/16 15:40:15, estark wrote: > mmenke: do you want to take another look before ...
5 years, 6 months ago (2015-06-16 15:56:08 UTC) #21
Ryan Sleevi
On 2015/06/16 15:56:08, mmenke wrote: > Moving ServeFromHosts() up just prevents users from mapping trusted ...
5 years, 6 months ago (2015-06-16 19:13:04 UTC) #22
mmenke
On 2015/06/16 19:13:04, Ryan Sleevi wrote: > On 2015/06/16 15:56:08, mmenke wrote: > > Moving ...
5 years, 6 months ago (2015-06-16 19:24:27 UTC) #23
Ryan Sleevi
On 2015/06/16 19:24:27, mmenke wrote: > It's not actually jerkware modifying those hosts file I'm ...
5 years, 6 months ago (2015-06-16 19:31:09 UTC) #24
estark
On 2015/06/16 19:31:09, Ryan Sleevi wrote: > On 2015/06/16 19:24:27, mmenke wrote: > > It's ...
5 years, 6 months ago (2015-06-18 23:25:10 UTC) #25
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1177933002/160001
5 years, 6 months ago (2015-06-19 03:09:28 UTC) #27
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromium_gn_compile_dbg/builds/83243)
5 years, 6 months ago (2015-06-19 03:22:07 UTC) #29
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1177933002/200001
5 years, 6 months ago (2015-06-19 03:35:12 UTC) #32
commit-bot: I haz the power
Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/22142)
5 years, 6 months ago (2015-06-19 04:12:31 UTC) #34
estark
On 2015/06/19 04:12:31, commit-bot: I haz the power wrote: > Try jobs failed on following ...
5 years, 6 months ago (2015-06-19 20:48:26 UTC) #35
Ryan Sleevi
On 2015/06/19 20:48:26, estark wrote: > I'm not sure what the correct behavior is and ...
5 years, 6 months ago (2015-06-19 21:22:43 UTC) #36
estark
On 2015/06/19 21:22:43, Ryan Sleevi wrote: > On 2015/06/19 20:48:26, estark wrote: > > I'm ...
5 years, 6 months ago (2015-06-19 22:27:55 UTC) #37
Ryan Sleevi
Patchset 11 LGTM. A nit, since you didn't CQ right away. https://codereview.chromium.org/1177933002/diff/220001/net/test/spawned_test_server/base_test_server.cc File net/test/spawned_test_server/base_test_server.cc (right): ...
5 years, 6 months ago (2015-06-19 23:08:16 UTC) #38
estark
https://codereview.chromium.org/1177933002/diff/220001/net/test/spawned_test_server/base_test_server.cc File net/test/spawned_test_server/base_test_server.cc (right): https://codereview.chromium.org/1177933002/diff/220001/net/test/spawned_test_server/base_test_server.cc#newcode239 net/test/spawned_test_server/base_test_server.cc:239: // Do an IPv4 lookup since kLocalhost is IPv4. ...
5 years, 6 months ago (2015-06-19 23:17:36 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1177933002/240001
5 years, 6 months ago (2015-06-19 23:26:11 UTC) #42
commit-bot: I haz the power
Committed patchset #12 (id:240001)
5 years, 6 months ago (2015-06-20 00:44:51 UTC) #43
commit-bot: I haz the power
5 years, 6 months ago (2015-06-20 00:45:52 UTC) #44
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/d1bc206e4635403d342e40d6e04845c029f1ada7
Cr-Commit-Position: refs/heads/master@{#335402}

Powered by Google App Engine
This is Rietveld 408576698