Skip to content

Commit

Permalink
HostResolverImpl: don't interpret NULL callback argument as a request…
Browse files Browse the repository at this point in the history 
… to do synchronous resolution.

BUG=90547,60149
TEST=net_unittests

Review URL: http://codereview.chromium.org/7520026

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@94552 0039d316-1c4b-4281-b951-d872f2087c98
  • Loading branch information
agayev@chromium.org committed Jul 28, 2011
1 parent a13cc36 commit 6e78dfb
Show file tree
Hide file tree
Showing 13 changed files with 127 additions and 158 deletions.
4 changes: 4 additions & 0 deletions jingle/notifier/communicator/xmpp_connection_generator_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ TEST_F(XmppConnectionGeneratorTest, DnsFailure) {
}

TEST_F(XmppConnectionGeneratorTest, DnsFailureSynchronous) {
MessageLoop message_loop;

EXPECT_CALL(delegate_, OnNewSettings(_)).Times(0);
EXPECT_CALL(delegate_, OnExhaustedSettings(_, _)).Times(1);

Expand All @@ -90,6 +92,8 @@ TEST_F(XmppConnectionGeneratorTest, ConnectionFailure) {
}

TEST_F(XmppConnectionGeneratorTest, ConnectionFailureSynchronous) {
MessageLoop message_loop;

EXPECT_CALL(delegate_, OnNewSettings(_)).
Times(5).
WillRepeatedly(
Expand Down
9 changes: 6 additions & 3 deletions net/base/host_resolver.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,13 +148,16 @@ class NET_API HostResolver {
// the sin(6)_port field of the sockaddr_in{6} struct. Returns OK if
// successful or an error code upon failure.
//
// When callback is null, the operation completes synchronously.
//
// When callback is non-null, the operation may be performed asynchronously.
// If the operation cannnot be completed synchronously, ERR_IO_PENDING will
// be returned and the real result code will be passed to the completion
// callback. Otherwise the result code is returned immediately from this
// call.
//
// When |callback| is null, there are two possibilities: either an IP
// address literal is being resolved or lookup should be performed from
// cache only, meaning info.only_use_cached_response() should be true; in
// both cases operation should complete synchronously.
//
// If |out_req| is non-NULL, then |*out_req| will be filled with a handle to
// the async request. This handle is not valid after the request has
// completed.
Expand Down
49 changes: 25 additions & 24 deletions net/base/host_resolver_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ namespace net {

namespace {

// Limit the size of hostnames that will be resolved to combat issues in
// some platform's resolvers.
const size_t kMaxHostLength = 4096;

// Helper to mutate the linked list contained by AddressList to the given
// port. Note that in general this is dangerous since the AddressList's
// data might be shared (and you should use AddressList::SetPort).
Expand Down Expand Up @@ -1143,6 +1147,19 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
// Update the net log and notify registered observers.
OnStartRequest(source_net_log, request_net_log, request_id, info);

// The result of |getaddrinfo| for empty hosts is inconsistent across systems.
// On Windows it gives the default interface's address, whereas on Linux it
// gives an error. We will make it fail on all platforms for consistency.
if (info.hostname().empty() || info.hostname().size() > kMaxHostLength) {
OnFinishRequest(source_net_log,
request_net_log,
request_id,
info,
ERR_NAME_NOT_RESOLVED,
0);
return ERR_NAME_NOT_RESOLVED;
}

// Build a key that identifies the request in the cache and in the
// outstanding jobs map.
Key key = GetEffectiveKeyForRequest(info);
Expand Down Expand Up @@ -1170,6 +1187,13 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
return net_error;
}

// Sanity check -- it shouldn't be the case that allow_cached_response is
// false while only_use_cached_response is true.
DCHECK(info.allow_cached_response() || !info.only_use_cached_response());

// If callback is NULL, we must be doing cache-only lookup.
DCHECK(callback || info.only_use_cached_response());

// If we have an unexpired cache entry, use it.
if (info.allow_cached_response() && cache_.get()) {
const HostCache::Entry* cache_entry = cache_->Lookup(
Expand Down Expand Up @@ -1201,30 +1225,7 @@ int HostResolverImpl::Resolve(const RequestInfo& info,
return ERR_NAME_NOT_RESOLVED;
}

// If no callback was specified, do a synchronous resolution.
if (!callback) {
AddressList addrlist;
int os_error = 0;
int error = ResolveAddrInfo(
effective_resolver_proc(), key.hostname, key.address_family,
key.host_resolver_flags, &addrlist, &os_error);
if (error == OK) {
MutableSetPort(info.port(), &addrlist);
*addresses = addrlist;
}

// Write to cache.
if (cache_.get())
cache_->Set(key, error, addrlist, base::TimeTicks::Now());

// Update the net log and notify registered observers.
OnFinishRequest(source_net_log, request_net_log, request_id, info, error,
os_error);

return error;
}

// Create a handle for this request, and pass it back to the user if they
// Create a handle for this request, and pass it back to the user if they
// asked for it (out_req != NULL).
Request* req = new Request(source_net_log, request_net_log, request_id, info,
callback, addresses);
Expand Down
111 changes: 15 additions & 96 deletions net/base/host_resolver_impl_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,41 +356,6 @@ class HostResolverImplTest : public testing::Test {
}
};

TEST_F(HostResolverImplTest, SynchronousLookup) {
AddressList addrlist;
const int kPortnum = 80;

scoped_refptr<RuleBasedHostResolverProc> resolver_proc(
new RuleBasedHostResolverProc(NULL));
resolver_proc->AddRule("just.testing", "192.168.1.42");

scoped_ptr<HostResolver> host_resolver(
CreateHostResolverImpl(resolver_proc));

HostResolver::RequestInfo info(HostPortPair("just.testing", kPortnum));
CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, log.bound());
EXPECT_EQ(OK, err);

CapturingNetLog::EntryList entries;
log.GetEntries(&entries);

EXPECT_EQ(2u, entries.size());
EXPECT_TRUE(LogContainsBeginEvent(
entries, 0, NetLog::TYPE_HOST_RESOLVER_IMPL));
EXPECT_TRUE(LogContainsEndEvent(
entries, 1, NetLog::TYPE_HOST_RESOLVER_IMPL));

const struct addrinfo* ainfo = addrlist.head();
EXPECT_EQ(static_cast<addrinfo*>(NULL), ainfo->ai_next);
EXPECT_EQ(sizeof(struct sockaddr_in), ainfo->ai_addrlen);

const struct sockaddr* sa = ainfo->ai_addr;
const struct sockaddr_in* sa_in = (const struct sockaddr_in*) sa;
EXPECT_TRUE(htons(kPortnum) == sa_in->sin_port);
EXPECT_TRUE(htonl(0xc0a8012a) == sa_in->sin_addr.s_addr);
}

TEST_F(HostResolverImplTest, AsynchronousLookup) {
AddressList addrlist;
const int kPortnum = 80;
Expand Down Expand Up @@ -570,7 +535,8 @@ TEST_F(HostResolverImplTest, EmptyHost) {
AddressList addrlist;
const int kPortnum = 5555;
HostResolver::RequestInfo info(HostPortPair("", kPortnum));
int err = host_resolver->Resolve(info, &addrlist, NULL, NULL, BoundNetLog());
int err = host_resolver->Resolve(info, &addrlist, NULL, NULL,
BoundNetLog());
EXPECT_EQ(ERR_NAME_NOT_RESOLVED, err);
}

Expand Down Expand Up @@ -990,7 +956,11 @@ TEST_F(HostResolverImplTest, Observers) {
// Resolve "host1".
HostResolver::RequestInfo info1(HostPortPair("host1", 70));
CapturingBoundNetLog log(CapturingNetLog::kUnbounded);
int rv = host_resolver->Resolve(info1, &addrlist, NULL, NULL, log.bound());
TestCompletionCallback callback;
int rv = host_resolver->Resolve(info1, &addrlist, &callback, NULL,
log.bound());
EXPECT_EQ(ERR_IO_PENDING, rv);
rv = callback.WaitForResult();
EXPECT_EQ(OK, rv);

CapturingNetLog::EntryList entries;
Expand All @@ -1012,7 +982,6 @@ TEST_F(HostResolverImplTest, Observers) {

// Resolve "host1" again -- this time it will be served from cache, but it
// should still notify of completion.
TestCompletionCallback callback;
rv = host_resolver->Resolve(info1, &addrlist, &callback, NULL, BoundNetLog());
ASSERT_EQ(OK, rv); // Should complete synchronously.

Expand All @@ -1027,7 +996,9 @@ TEST_F(HostResolverImplTest, Observers) {
// Resolve "host2", setting referrer to "http://foobar.com"
HostResolver::RequestInfo info2(HostPortPair("host2", 70));
info2.set_referrer(GURL("http://foobar.com"));
rv = host_resolver->Resolve(info2, &addrlist, NULL, NULL, BoundNetLog());
rv = host_resolver->Resolve(info2, &addrlist, &callback, NULL, BoundNetLog());
EXPECT_EQ(ERR_IO_PENDING, rv);
rv = callback.WaitForResult();
EXPECT_EQ(OK, rv);

EXPECT_EQ(3U, observer.start_log.size());
Expand All @@ -1043,7 +1014,7 @@ TEST_F(HostResolverImplTest, Observers) {

// Resolve "host3"
HostResolver::RequestInfo info3(HostPortPair("host3", 70));
host_resolver->Resolve(info3, &addrlist, NULL, NULL, BoundNetLog());
host_resolver->Resolve(info3, &addrlist, &callback, NULL, BoundNetLog());

// No effect this time, since observer was removed.
EXPECT_EQ(3U, observer.start_log.size());
Expand Down Expand Up @@ -1648,61 +1619,6 @@ TEST_F(HostResolverImplTest, SetDefaultAddressFamily_IPv6) {
EXPECT_EQ("192.2.104.1", NetAddressToString(addrlist[2].head()));
}

// This tests that the default address family is respected for synchronous
// resolutions.
TEST_F(HostResolverImplTest, SetDefaultAddressFamily_Synchronous) {
scoped_refptr<CapturingHostResolverProc> resolver_proc(
new CapturingHostResolverProc(new EchoingHostResolverProc));

scoped_ptr<HostResolverImpl> host_resolver(new HostResolverImpl(
resolver_proc, HostCache::CreateDefaultCache(), kMaxJobs,
kMaxRetryAttempts, NULL));

host_resolver->SetDefaultAddressFamily(ADDRESS_FAMILY_IPV4);

// Unblock the resolver thread so the requests can run.
resolver_proc->Signal();

HostResolver::RequestInfo req[] = {
CreateResolverRequestForAddressFamily("b", MEDIUM,
ADDRESS_FAMILY_UNSPECIFIED),
CreateResolverRequestForAddressFamily("b", MEDIUM, ADDRESS_FAMILY_IPV6),
CreateResolverRequestForAddressFamily("b", MEDIUM,
ADDRESS_FAMILY_UNSPECIFIED),
CreateResolverRequestForAddressFamily("b", MEDIUM, ADDRESS_FAMILY_IPV4),
};
AddressList addrlist[arraysize(req)];

// Start and run all of the requests synchronously.
for (size_t i = 0; i < arraysize(req); ++i) {
int rv = host_resolver->Resolve(req[i], &addrlist[i],
NULL, NULL, BoundNetLog());
EXPECT_EQ(OK, rv) << i;
}

// We should have sent 2 requests to the resolver --
// one for (b, IPv4), and one for (b, IPv6).
CapturingHostResolverProc::CaptureList capture_list =
resolver_proc->GetCaptureList();
ASSERT_EQ(2u, capture_list.size());

EXPECT_EQ("b", capture_list[0].hostname);
EXPECT_EQ(ADDRESS_FAMILY_IPV4, capture_list[0].address_family);

EXPECT_EQ("b", capture_list[1].hostname);
EXPECT_EQ(ADDRESS_FAMILY_IPV6, capture_list[1].address_family);

// Now check that the correct resolved IP addresses were returned.
// Addresses take the form: 192.x.y.z
// x = length of hostname
// y = ASCII value of hostname[0]
// z = value of address family
EXPECT_EQ("192.1.98.1", NetAddressToString(addrlist[0].head()));
EXPECT_EQ("192.1.98.2", NetAddressToString(addrlist[1].head()));
EXPECT_EQ("192.1.98.1", NetAddressToString(addrlist[2].head()));
EXPECT_EQ("192.1.98.1", NetAddressToString(addrlist[3].head()));
}

TEST_F(HostResolverImplTest, DisallowNonCachedResponses) {
AddressList addrlist;
const int kPortnum = 80;
Expand All @@ -1723,7 +1639,10 @@ TEST_F(HostResolverImplTest, DisallowNonCachedResponses) {

// This time, we fetch normally.
info.set_only_use_cached_response(false);
err = host_resolver->Resolve(info, &addrlist, NULL, NULL, log.bound());
TestCompletionCallback callback;
err = host_resolver->Resolve(info, &addrlist, &callback, NULL, log.bound());
EXPECT_EQ(ERR_IO_PENDING, err);
err = callback.WaitForResult();
EXPECT_EQ(OK, err);

// Now we should be able to fetch from the cache.
Expand Down
13 changes: 0 additions & 13 deletions net/base/host_resolver_proc.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,22 +122,9 @@ int SystemHostResolverProc(const std::string& host,
HostResolverFlags host_resolver_flags,
AddressList* addrlist,
int* os_error) {
static const size_t kMaxHostLength = 4096;

if (os_error)
*os_error = 0;

// The result of |getaddrinfo| for empty hosts is inconsistent across systems.
// On Windows it gives the default interface's address, whereas on Linux it
// gives an error. We will make it fail on all platforms for consistency.
if (host.empty())
return ERR_NAME_NOT_RESOLVED;

// Limit the size of hostnames that will be resolved to combat issues in some
// platform's resolvers.
if (host.size() > kMaxHostLength)
return ERR_NAME_NOT_RESOLVED;

struct addrinfo* ai = NULL;
struct addrinfo hints = {0};

Expand Down
Loading

0 comments on commit 6e78dfb

Please sign in to comment.