[net/dns] Handle TC bit on DNS response in DnsTransaction.

[net/dns] Handle TC bit on DNS response in DnsTransaction.

Adds DnsTCPAttempt.

This implementation does not reuse TCP sockets nor share them across
concurrent DNS over TCP transactions.

BUG=116785
TEST=net_unittests --gtest_filter=DnsTransactionTest.TCP*


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=174227

[mmenke, 2012-12-19 16:44:09]

Sorry I'm a bit slow this week.  Looks really good to me.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.h
File net/dns/dns_response.h (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.h#ne...
net/dns/dns_response.h:101: // Construct a response buffer of given length. Used
for TCP transactions.
nit:  For consistency, should start all constructor comments with same form of
"Construct".

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.cc
File net/dns/dns_socket_pool.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.c...
net/dns/dns_socket_pool.cc:62: scoped_ptr<StreamSocket>
DnsSocketPool::AllocateTCPSocket(
Have you thought about using a TransportClientSocketPool?  It has a bit more
than we need, but should still do the trick.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.h
File net/dns/dns_socket_pool.h (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.h...
net/dns/dns_socket_pool.h:64: // TODO(szym): add comment
Assume you're going to take care of that in this CL.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:78: // and calls |callback| upon completion.
There's no |callback| defined here.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:89: virtual unsigned server_index() const = 0;
Virtual methods should use CamelCase.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:89: virtual unsigned server_index() const = 0;
While these methods do fairly obvious things, still think it's a good idea to
document virtual methods.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:116: 
nit:  "DnsAttempt:" or "DnsAttempt implementation:"

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:282: 
nit:  "DnsAttempt:" or "DnsAttempt implementation:"

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:350: if (rv == OK) {
Maybe a DCHECK_EQ(STATE_NONE, next_state_);?

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:422: // Allocate more space so that DnsResponse's
sanity check passes.
Which check is that?

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:423: response_.reset(new DnsResponse(response_length_
+ 1));
That +1 could trigger overflow.  Also, may want to do a simple sanity check - if
the response length is less than the query length, can just fail.  May be worth
having a test for this, too.

May also want to use a hard-coded size limit, though suppose a 2^16 limit isn't
too many times greater than the largest reasonable size limit.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:423: response_.reset(new DnsResponse(response_length_
+ 1));
Speaking of which, we may want to fuzz test this, too, though I think
response_length_ is the only new possible attack surface area we provide, not
present in the UDP version.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:687: // Cancel all other attempts, no point waiting
on them.
No point, even if there are multiple servers?

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:781: result = MakeTCPAttempt(result.attempt);
Should we be remembering this somewhere?  Or might a server just require TCP/IP
for longer responses?  No need to worry about it in this CL.

[mmenke, 2012-12-19 16:58:46]

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:423: response_.reset(new DnsResponse(response_length_
+ 1));
On 2012/12/19 16:44:09, Matt Menke wrote:
> That +1 could trigger overflow.  Also, may want to do a simple sanity check -
if
> the response length is less than the query length, can just fail.  May be
worth
> having a test for this, too.
> 
> May also want to use a hard-coded size limit, though suppose a 2^16 limit
isn't
> too many times greater than the largest reasonable size limit.

Oops - that "test" comment was meant for the second paragraph - a length of
2^16-1.

[szym, 2012-12-19 17:01:17]

Thanks for the review.

Some explanatory comments for now.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:422: // Allocate more space so that DnsResponse's
sanity check passes.
On 2012/12/19 16:44:09, Matt Menke wrote:
> Which check is that?

size check for |nbytes| in DnsResponse::InitParse.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:423: response_.reset(new DnsResponse(response_length_
+ 1));
On 2012/12/19 16:44:09, Matt Menke wrote:
> That +1 could trigger overflow. 

I don't think so. Because |response_length_| will be promoted and there will be
no truncation because it is DnsResponse(size_t).

> Also, may want to do a simple sanity check - if
> the response length is less than the query length, can just fail.  May be
worth
> having a test for this, too.

Good idea. We already have a check for that in DnsResponse::InitParse, but this
would allow earlier abort.

> May also want to use a hard-coded size limit, though suppose a 2^16 limit
isn't
> too many times greater than the largest reasonable size limit.

I think 2^16 is a reasonable limit, I'd add a comment if it's not obvious that
there is a limit.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:687: // Cancel all other attempts, no point waiting
on them.
On 2012/12/19 16:44:09, Matt Menke wrote:
> No point, even if there are multiple servers?

We don't really anticipate getting substantially different results from
different servers, so if the first response we got says it's truncated (TC),
then we shouldn't hope we will get a non-TC response from the other servers.

The only rationale would be that other servers could be quicker in responding,
but to have any confidence that one server is faster than another we either need
to keep track of their past performance or we run races.

I'd rather keep the logic simple for now and assume we make **at most one** TCP
attempt per DnsTransaction.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:781: result = MakeTCPAttempt(result.attempt);
On 2012/12/19 16:44:09, Matt Menke wrote:
> Should we be remembering this somewhere?  Or might a server just require
TCP/IP
> for longer responses?  No need to worry about it in this CL.

This is characteristic to the query more than the server, so we could cache it
in HostCache as a small optimization. However, given that <1% queries need TCP,
I wouldn't bother at this time.

[mmenke, 2012-12-19 17:13:50]

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:423: response_.reset(new DnsResponse(response_length_
+ 1));
On 2012/12/19 17:01:17, szym wrote:
> On 2012/12/19 16:44:09, Matt Menke wrote:
> > That +1 could trigger overflow. 
> 
> I don't think so. Because |response_length_| will be promoted and there will
be
> no truncation because it is DnsResponse(size_t).

You're right - I had been incorrectly assuming promotion would occur at the
point of assignment.

> > Also, may want to do a simple sanity check - if
> > the response length is less than the query length, can just fail.  May be
> worth
> > having a test for this, too.
> 
> Good idea. We already have a check for that in DnsResponse::InitParse, but
this
> would allow earlier abort.
> 
> > May also want to use a hard-coded size limit, though suppose a 2^16 limit
> isn't
> > too many times greater than the largest reasonable size limit.
> 
> I think 2^16 is a reasonable limit, I'd add a comment if it's not obvious that
> there is a limit.

I think we can live without it.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:781: result = MakeTCPAttempt(result.attempt);
On 2012/12/19 17:01:17, szym wrote:
> On 2012/12/19 16:44:09, Matt Menke wrote:
> > Should we be remembering this somewhere?  Or might a server just require
> TCP/IP
> > for longer responses?  No need to worry about it in this CL.
> 
> This is characteristic to the query more than the server, so we could cache it
> in HostCache as a small optimization. However, given that <1% queries need
TCP,
> I wouldn't bother at this time.

If it's just for the query, I agree that it's not worth it.  Wasn't sure if
there were DNS servers out there that insist on TCP or not.

[mmenke, 2012-12-19 17:15:47]

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.cc#n...
net/dns/dns_response.cc:152: if (nbytes < query.io_buffer()->size() || nbytes >=
io_buffer_->size())
Ahh...So why are you using ">=" rather than ">"?

[szym, 2012-12-19 18:16:24]

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.cc#n...
net/dns/dns_response.cc:152: if (nbytes < query.io_buffer()->size() || nbytes >=
io_buffer_->size())
On 2012/12/19 17:15:48, Matt Menke wrote:
> Ahh...So why are you using ">=" rather than ">"?

If this check was ">", then there is no way to catch overflow on UDP since we
would never be able to read more into the buffer than the size.

[mmenke, 2012-12-19 18:21:29]

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.cc
File net/dns/dns_response.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.cc#n...
net/dns/dns_response.cc:152: if (nbytes < query.io_buffer()->size() || nbytes >=
io_buffer_->size())
On 2012/12/19 18:16:24, szym wrote:
> On 2012/12/19 17:15:48, Matt Menke wrote:
> > Ahh...So why are you using ">=" rather than ">"?
> 
> If this check was ">", then there is no way to catch overflow on UDP since we
> would never be able to read more into the buffer than the size.

Thanks for the explanation.

[szym, 2012-12-19 23:35:56]

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.h
File net/dns/dns_response.h (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_response.h#ne...
net/dns/dns_response.h:101: // Construct a response buffer of given length. Used
for TCP transactions.
On 2012/12/19 16:44:09, Matt Menke wrote:
> nit:  For consistency, should start all constructor comments with same form of
> "Construct".

Done.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.cc
File net/dns/dns_socket_pool.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.c...
net/dns/dns_socket_pool.cc:62: scoped_ptr<StreamSocket>
DnsSocketPool::AllocateTCPSocket(
On 2012/12/19 16:44:09, Matt Menke wrote:
> Have you thought about using a TransportClientSocketPool?  It has a bit more
> than we need, but should still do the trick.

Right now I don't really see the benefit beyond saving the need to Connect the
socket. If we ever start reusing TCP sockets, or perhaps even maintaining a
persistent connection for high-throughput (although high-latency) resolving,
then TransportClientSocketPool will fit the bill.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.h
File net/dns/dns_socket_pool.h (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.h...
net/dns/dns_socket_pool.h:64: // TODO(szym): add comment
On 2012/12/19 16:44:09, Matt Menke wrote:
> Assume you're going to take care of that in this CL.

Done.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:78: // and calls |callback| upon completion.
On 2012/12/19 16:44:09, Matt Menke wrote:
> There's no |callback| defined here.

Changed so that CompletionCallback is passed to Start.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:89: virtual unsigned server_index() const = 0;
On 2012/12/19 16:44:09, Matt Menke wrote:
> While these methods do fairly obvious things, still think it's a good idea to
> document virtual methods.

Done. Done.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:116: 
On 2012/12/19 16:44:09, Matt Menke wrote:
> nit:  "DnsAttempt:" or "DnsAttempt implementation:"

Done.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:282: 
On 2012/12/19 16:44:09, Matt Menke wrote:
> nit:  "DnsAttempt:" or "DnsAttempt implementation:"

Done.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:350: if (rv == OK) {
On 2012/12/19 16:44:09, Matt Menke wrote:
> Maybe a DCHECK_EQ(STATE_NONE, next_state_);?

Done.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:423: response_.reset(new DnsResponse(response_length_
+ 1));
On 2012/12/19 17:01:17, szym wrote:
> On 2012/12/19 16:44:09, Matt Menke wrote:
> > That +1 could trigger overflow. 
> 
> I don't think so. Because |response_length_| will be promoted and there will
be
> no truncation because it is DnsResponse(size_t).
> 
> > Also, may want to do a simple sanity check - if
> > the response length is less than the query length, can just fail.  May be
> worth
> > having a test for this, too.
> 
> Good idea. We already have a check for that in DnsResponse::InitParse, but
this
> would allow earlier abort.
> 
> > May also want to use a hard-coded size limit, though suppose a 2^16 limit
> isn't
> > too many times greater than the largest reasonable size limit.
> 
> I think 2^16 is a reasonable limit, I'd add a comment if it's not obvious that
> there is a limit.

Done.

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:426: // Check if advertised response is too short.
(Optimization only.)
This comment is meant to reflect that if we removed this check, the semantic
behavior would not change (we would just close the socket earlier).

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:798: !had_tcp_attempt_) {
PTAL. Logic change.

[mmenke, 2012-12-20 16:26:27]

LGTM!  Just very minor comments.

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.cc
File net/dns/dns_socket_pool.cc (right):

https://codereview.chromium.org/11567031/diff/14003/net/dns/dns_socket_pool.c...
net/dns/dns_socket_pool.cc:62: scoped_ptr<StreamSocket>
DnsSocketPool::AllocateTCPSocket(
On 2012/12/19 23:35:56, szym wrote:
> On 2012/12/19 16:44:09, Matt Menke wrote:
> > Have you thought about using a TransportClientSocketPool?  It has a bit more
> > than we need, but should still do the trick.
> 
> Right now I don't really see the benefit beyond saving the need to Connect the
> socket. If we ever start reusing TCP sockets, or perhaps even maintaining a
> persistent connection for high-throughput (although high-latency) resolving,
> then TransportClientSocketPool will fit the bill.

I hadn't realized the odds of subsequent queries requiring TCP/IP queries were
independent, so was thinking in terms of socket reuse.

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:97: Value* NetLogResponseCallback(NetLog::LogLevel)
const {
Per Google style guide:

"Always have named parameters in the declaration"

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:250: CHECK(GetResponse());
Do we really need a CHECK here?  Same goes for the new one.  Seems like we're
only checking that DnsResponse::InitParse returns true only when it populates
its parser_ field, which doesn't really seem to warrant a CHECK.

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:798: !had_tcp_attempt_) {
On 2012/12/19 23:35:57, szym wrote:
> PTAL. Logic change.

Nice catch.  Think this is worth a short comment about allowing unexpected
responses in the UDP case only.

[szym, 2012-12-20 18:14:21]

Thanks!

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.cc
File net/dns/dns_transaction.cc (right):

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:97: Value* NetLogResponseCallback(NetLog::LogLevel)
const {
On 2012/12/20 16:26:28, Matt Menke wrote:
> Per Google style guide:
> 
> "Always have named parameters in the declaration"

Done.

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:250: CHECK(GetResponse());
On 2012/12/20 16:26:28, Matt Menke wrote:
> Do we really need a CHECK here?  Same goes for the new one.  Seems like we're
> only checking that DnsResponse::InitParse returns true only when it populates
> its parser_ field, which doesn't really seem to warrant a CHECK.

We don't. It's a left-over from trying to diagnose an old crasher. Removed.

https://codereview.chromium.org/11567031/diff/27002/net/dns/dns_transaction.c...
net/dns/dns_transaction.cc:798: !had_tcp_attempt_) {
On 2012/12/20 16:26:28, Matt Menke wrote:
> On 2012/12/19 23:35:57, szym wrote:
> > PTAL. Logic change.
> 
> Nice catch.  Think this is worth a short comment about allowing unexpected
> responses in the UDP case only.

Done.

[commit-bot: I haz the power, 2012-12-20 18:30:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/szym@chromium.org/11567031/36002

[commit-bot: I haz the power, 2012-12-20 20:45:16]

Change committed as 174227