Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
938093003:
Always treat .localhost as loopback (Closed)
DescriptionAlways treat .localhost as loopback
This means that .localhost is always redirected to localhost in DNS
lookups, and |net::IsLocalhost| returns true for .localhost hostnames.
BUG=455825
Committed: https://crrev.com/5d131a1fd9b808c5fd08c45f8299e669b13ec393
Cr-Commit-Position: refs/heads/master@{#322452}
Patch Set 1 #Patch Set 2 : add comments #
Total comments: 8
Patch Set 3 : rsleevi's comments #
Total comments: 3
Patch Set 4 : move localhost rewriting up to HostResolverImpl #
Total comments: 6
Patch Set 5 : rsleevi's comments #
Total comments: 4
Patch Set 6 : rebase #Patch Set 7 : move constants, add a test case, style fix #
Total comments: 2
Messages
Total messages: 24 (4 generated)
estark@chromium.org changed reviewers: + rsleevi@chromium.org
rsleevi@, could you please take a look and help me figure out if this is on the right track? This is in response to crbug.com/455825. My questions include: 1. Does this look like the right general approach? 2. Any pointers for how to test the changes in host_resolver_proc.cc and dns_transaction.cc?
rsleevi@chromium.org changed reviewers: + ttuttle@chromium.org - rsleevi@chromium.org
I'm pretty sure Thomas is the DNS expert these days, probably best to have him take a pass on this. Plus, he's quicker at reviewing than I am :) https://codereview.chromium.org/938093003/diff/20001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/20001/net/base/net_util.cc#new... net/base/net_util.cc:1021: bool IsLocalhostTLD(const std::string& host) { I guess it depends on the rules for |host| here, but it's valid to have a trailing dot in DNS to indicate end-label that is "localhost.localhost." is valid and equivalent to "localhost.localhost" (the . traditionally signals to OS libraries that have suffix search orders not to execute them) This is probably a bug that we don't handle these on line 987-991, but it really depends on the API contract we want. I'm pretty sure these functions will receive . suffices as inputs, so we should probably handle them. https://codereview.chromium.org/938093003/diff/20001/net/base/net_util.cc#new... net/base/net_util.cc:1022: int host_length = host.length(); s/int/size_t/ - right numeric for the job https://codereview.chromium.org/938093003/diff/20001/net/base/net_util_unitte... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/938093003/diff/20001/net/base/net_util_unitte... net/base/net_util_unittest.cc:816: EXPECT_FALSE(net::IsLocalhostTLD("foo.localhos")); Add for the .suffix case foo.localhost. https://codereview.chromium.org/938093003/diff/20001/net/dns/dns_transaction.cc File net/dns/dns_transaction.cc (right): https://codereview.chromium.org/938093003/diff/20001/net/dns/dns_transaction.... net/dns/dns_transaction.cc:571: hostname_ = "localhost"; No clue if this is correct; This seems wrong/weird, in as much as GetHostname() will now return a different name than the input.
thanks rsleevi! https://codereview.chromium.org/938093003/diff/20001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/20001/net/base/net_util.cc#new... net/base/net_util.cc:1021: bool IsLocalhostTLD(const std::string& host) { On 2015/02/19 22:24:56, Ryan Sleevi wrote: > I guess it depends on the rules for |host| here, but it's valid to have a > trailing dot in DNS to indicate end-label > > that is > > "localhost.localhost." is valid and equivalent to "localhost.localhost" (the . > traditionally signals to OS libraries that have suffix search orders not to > execute them) > > This is probably a bug that we don't handle these on line 987-991, but it really > depends on the API contract we want. I'm pretty sure these functions will > receive . suffices as inputs, so we should probably handle them. Gotcha. Done in this function, and filed another bug for lines 987-991: crbug.com/460331 https://codereview.chromium.org/938093003/diff/20001/net/base/net_util.cc#new... net/base/net_util.cc:1022: int host_length = host.length(); On 2015/02/19 22:24:56, Ryan Sleevi wrote: > s/int/size_t/ - right numeric for the job Done. https://codereview.chromium.org/938093003/diff/20001/net/base/net_util_unitte... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/938093003/diff/20001/net/base/net_util_unitte... net/base/net_util_unittest.cc:816: EXPECT_FALSE(net::IsLocalhostTLD("foo.localhos")); On 2015/02/19 22:24:56, Ryan Sleevi wrote: > Add for the .suffix case > > foo.localhost. Done. https://codereview.chromium.org/938093003/diff/20001/net/dns/dns_transaction.cc File net/dns/dns_transaction.cc (right): https://codereview.chromium.org/938093003/diff/20001/net/dns/dns_transaction.... net/dns/dns_transaction.cc:571: hostname_ = "localhost"; On 2015/02/19 22:24:56, Ryan Sleevi wrote: > No clue if this is correct; This seems wrong/weird, in as much as GetHostname() > will now return a different name than the input. Ah, I see what you mean. I changed it to use localhost in |PrepareSearch|, but I'm not sure if that's correct either.
ttuttle, do you think you could take a look when you have a chance? thanks!
On 2015/02/23 18:47:07, emily wrote: > ttuttle, do you think you could take a look when you have a chance? thanks! Looking now!
The hostname-rewriting (*.localhost => "localhost.") logic looks good to me, but I think you're rewriting one layer too low to use the localhost entry in /etc/hosts when we're using the internal (async) DNS resolver. Take a look at HostResolverImpl::ServeFromHosts in net/dns/host_resolver_impl.cc -- it's called from ResolveHelper, which is called to check the cache and hosts file before creating a DnsTask to resolve the name. It looks like GetEffectiveKeyForRequest might be the best spot to flatten the localhost names into "localhost.", but you'd have to see if there are any untoward side effects of doing it there. Test-wise: 1. For the internal async resolver (DnsTransaction), I *think* the MockUDPClientSockets will abort if they run out of data to send/receive, so you should be okay writing a test that tries to resolve domains you expect to match localhost and asserts that they don't actually result in a query sent over the network. 2. I'm not really sure how to test all the way down to the OS resolver -- we don't seem to have unit tests for HostResolverProc. 3. That said, if you move the rewriting into HostResolverImpl, you may be able to test both code paths more easily by mocking out the underlying transactions and testing just the rewriting. I'd still leave in the tests of the actual async resolver behavior, since they test the desired behavior (no query sent over the network) instead of just the rewriting bit. https://codereview.chromium.org/938093003/diff/40001/net/dns/dns_transaction.cc File net/dns/dns_transaction.cc (right): https://codereview.chromium.org/938093003/diff/40001/net/dns/dns_transaction.... net/dns/dns_transaction.cc:620: // Redirect .localhost queries to localhost to make sure that they are Nit: "to localhost.", since you're including the trailing dot. https://codereview.chromium.org/938093003/diff/40001/net/dns/host_resolver_pr... File net/dns/host_resolver_proc.cc (right): https://codereview.chromium.org/938093003/diff/40001/net/dns/host_resolver_pr... net/dns/host_resolver_proc.cc:194: // Redirect .localhost queries to localhost to make sure that they are Nit: ditto.
Thanks ttuttle! I did some of what you suggested (patch set 4) but ran into some confusion -- see some questions inline. (Sorry -- noogler here and might be just a tad bit in over my head) On 2015/02/24 16:39:23, ttuttle wrote: > The hostname-rewriting (*.localhost => "localhost.") logic looks good to me, but > I think you're rewriting one layer too low to use the localhost entry in > /etc/hosts when we're using the internal (async) DNS resolver. > > Take a look at HostResolverImpl::ServeFromHosts in net/dns/host_resolver_impl.cc > -- it's called from ResolveHelper, which is called to check the cache and hosts > file before creating a DnsTask to resolve the name. > > It looks like GetEffectiveKeyForRequest might be the best spot to flatten the > localhost names into "localhost.", but you'd have to see if there are any > untoward side effects of doing it there. There is one untoward side effect in that this change breaks a DCHECK when a foo.localhost lookup is done from a unit test. I got around this by DCHECK'ing something different when it's a .localhost query (see host_resolver_imp.cc:1277), but this feels a bit wrong. > > Test-wise: > > 1. For the internal async resolver (DnsTransaction), I *think* the > MockUDPClientSockets will abort if they run out of data to send/receive, so you > should be okay writing a test that tries to resolve domains you expect to match > localhost and asserts that they don't actually result in a query sent over the > network. So when I started poking around a little more in DnsTransaction, I became unsure that it's possible to write such a test: it looks to me like DnsTransaction always sends the query out on the network, and HostResolverImpl is responsible for not creating a DnsTransaction if the query shouldn't be sent out. (I did manually test my original patch by watching DNS queries on tcpdump, and I thought that the patch worked, i.e. rewriting *.localhost in DnsTransaction stopped the query from going out on the network... but maybe I was confused by some caching going on somewhere?) > > 2. I'm not really sure how to test all the way down to the OS resolver -- we > don't seem to have unit tests for HostResolverProc. > > 3. That said, if you move the rewriting into HostResolverImpl, you may be able > to test both code paths more easily by mocking out the underlying transactions > and testing just the rewriting. I'd still leave in the tests of the actual async > resolver behavior, since they test the desired behavior (no query sent over the > network) instead of just the rewriting bit. I added a test for HostResolverImpl but wasn't 100% sure that it was along the liens of what you had in mind -- could you please take a look and let me know? > > https://codereview.chromium.org/938093003/diff/40001/net/dns/dns_transaction.cc > File net/dns/dns_transaction.cc (right): > > https://codereview.chromium.org/938093003/diff/40001/net/dns/dns_transaction.... > net/dns/dns_transaction.cc:620: // Redirect .localhost queries to localhost to > make sure that they are > Nit: "to localhost.", since you're including the trailing dot. > > https://codereview.chromium.org/938093003/diff/40001/net/dns/host_resolver_pr... > File net/dns/host_resolver_proc.cc (right): > > https://codereview.chromium.org/938093003/diff/40001/net/dns/host_resolver_pr... > net/dns/host_resolver_proc.cc:194: // Redirect .localhost queries to localhost > to make sure that they are > Nit: ditto.
https://codereview.chromium.org/938093003/diff/40001/net/dns/dns_transaction.cc File net/dns/dns_transaction.cc (right): https://codereview.chromium.org/938093003/diff/40001/net/dns/dns_transaction.... net/dns/dns_transaction.cc:620: // Redirect .localhost queries to localhost to make sure that they are On 2015/02/24 16:39:23, ttuttle wrote: > Nit: "to localhost.", since you're including the trailing dot. fixed in the place where this comment has been relocated to
Just more drive-bys while I let Thomas review :) https://codereview.chromium.org/938093003/diff/60001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:148: static const size_t kLocalhostTLDLength = strlen(kLocalhostTLD); DANGER: strlen is a function call, which violates http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static-... Now, the compiler is typically smart enough to treat strlen as a special case (due it it going into an intrinsic), and then further optimize this as a const, but it's not guaranteed. arraysize(kLocalhostTLD) - 1 is a compile-time constant. https://codereview.chromium.org/938093003/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:1021: bool IsLocalhostTLD(const std::string& host) { You can avoid the double comparison if (host.empty()) return false; size_t host_len = host.size(); if (*host.rbegin() == '.') --host_len; if (host_len < kLocalhostTLDLength) return false; const char* host = host.data() + host_len - kLocalhostTLDLength; return base::strncasecmp(host, kLocalhostTLD, kLocalhostTLDLength) == 0; https://codereview.chromium.org/938093003/diff/60001/net/dns/host_resolver_im... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/938093003/diff/60001/net/dns/host_resolver_im... net/dns/host_resolver_impl.cc:2206: if (IsLocalhostTLD(info.hostname())) { no braces around single-line conditionals in //net (Google style says either/or, but follow local convention. //net outside of //quic doesn't use em)
thanks rsleevi! https://codereview.chromium.org/938093003/diff/60001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:148: static const size_t kLocalhostTLDLength = strlen(kLocalhostTLD); On 2015/02/27 03:21:10, Ryan Sleevi wrote: > DANGER: strlen is a function call, which violates > http://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Static-... > > Now, the compiler is typically smart enough to treat strlen as a special case > (due it it going into an intrinsic), and then further optimize this as a const, > but it's not guaranteed. > > arraysize(kLocalhostTLD) - 1 is a compile-time constant. Done. https://codereview.chromium.org/938093003/diff/60001/net/base/net_util.cc#new... net/base/net_util.cc:1021: bool IsLocalhostTLD(const std::string& host) { On 2015/02/27 03:21:10, Ryan Sleevi wrote: > You can avoid the double comparison > > if (host.empty()) > return false; > > size_t host_len = host.size(); > if (*host.rbegin() == '.') > --host_len; > if (host_len < kLocalhostTLDLength) > return false; > > const char* host = host.data() + host_len - kLocalhostTLDLength; > > return base::strncasecmp(host, kLocalhostTLD, kLocalhostTLDLength) == 0; Done. https://codereview.chromium.org/938093003/diff/60001/net/dns/host_resolver_im... File net/dns/host_resolver_impl.cc (right): https://codereview.chromium.org/938093003/diff/60001/net/dns/host_resolver_im... net/dns/host_resolver_impl.cc:2206: if (IsLocalhostTLD(info.hostname())) { On 2015/02/27 03:21:10, Ryan Sleevi wrote: > no braces around single-line conditionals in //net > > (Google style says either/or, but follow local convention. //net outside of > //quic doesn't use em) Done.
rsleevi@chromium.org changed reviewers: + rsleevi@chromium.org
ttuttle: Ping? :)
ttuttle: Ping Emily: Feel free to keep pinging Thomas every day :) https://codereview.chromium.org/938093003/diff/80001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/80001/net/base/net_util.cc#new... net/base/net_util.cc:148: static const size_t kLocalhostTLDLength = arraysize(kLocalhostTLD) - 1; Turns out you could move these to line 1024, since you never use this outside this scope. https://codereview.chromium.org/938093003/diff/80001/net/base/net_util_unitte... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/938093003/diff/80001/net/base/net_util_unitte... net/base/net_util_unittest.cc:827: EXPECT_FALSE(net::IsLocalhostTLD("foo.localhost.com")); foo.localhoste ? Just want to make sure we cover all cases
Heh, ok, since Ryan said to.... ttuttle: ping! https://codereview.chromium.org/938093003/diff/80001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/80001/net/base/net_util.cc#new... net/base/net_util.cc:148: static const size_t kLocalhostTLDLength = arraysize(kLocalhostTLD) - 1; On 2015/03/24 23:15:20, Ryan Sleevi wrote: > Turns out you could move these to line 1024, since you never use this outside > this scope. Done. https://codereview.chromium.org/938093003/diff/80001/net/base/net_util_unitte... File net/base/net_util_unittest.cc (right): https://codereview.chromium.org/938093003/diff/80001/net/base/net_util_unitte... net/base/net_util_unittest.cc:827: EXPECT_FALSE(net::IsLocalhostTLD("foo.localhost.com")); On 2015/03/24 23:15:20, Ryan Sleevi wrote: > foo.localhoste ? > > Just want to make sure we cover all cases Done.
Pong! Looking at this now.
LGTM with one nit, which you are free to ignore. https://codereview.chromium.org/938093003/diff/120001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/120001/net/base/net_util.cc#ne... net/base/net_util.cc:1024: if (*host.rbegin() == '.') Nit: host.back()? I think we're using C++11 now, so it should be okay.
https://codereview.chromium.org/938093003/diff/120001/net/base/net_util.cc File net/base/net_util.cc (right): https://codereview.chromium.org/938093003/diff/120001/net/base/net_util.cc#ne... net/base/net_util.cc:1024: if (*host.rbegin() == '.') On 2015/03/26 17:04:36, ttuttle wrote: > Nit: host.back()? I think we're using C++11 now, so it should be okay. language, not library, so rbegin is needed
On 2015/03/26 17:10:57, Ryan Sleevi wrote: > https://codereview.chromium.org/938093003/diff/120001/net/base/net_util.cc > File net/base/net_util.cc (right): > > https://codereview.chromium.org/938093003/diff/120001/net/base/net_util.cc#ne... > net/base/net_util.cc:1024: if (*host.rbegin() == '.') > On 2015/03/26 17:04:36, ttuttle wrote: > > Nit: host.back()? I think we're using C++11 now, so it should be okay. > > language, not library, so rbegin is needed Alright, rbegin it is.
The CQ bit was checked by estark@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/938093003/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/5d131a1fd9b808c5fd08c45f8299e669b13ec393 Cr-Commit-Position: refs/heads/master@{#322452} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
